Skip to content

feat(memory): model memory as a userland extension — implements #3537#5205

Open
BenKurrek wants to merge 29 commits into
mainfrom
reborn/memory-lift-followups
Open

feat(memory): model memory as a userland extension — implements #3537#5205
BenKurrek wants to merge 29 commits into
mainfrom
reborn/memory-lift-followups

Conversation

@BenKurrek

@BenKurrek BenKurrek commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Reborn: model memory as a userland extension — implements #3537

Implements the bulk of #3537 — Extension Manifest v2 architecture, source-aware trust, host-defined capability profiles + conformance, the memory profile-binding policy, and the always-on native document-store provider. It also includes a config-driven third-party document-store provider backed by a self-hosted, fully-local mem0 (mem0 OSS on localhost — no api.mem0.ai, optional key), demonstrating the provider is swappable entirely through config with no hardcoded native assumption (the host_runtime resolver names no concrete provider; construction follows the ironclaw_embeddings::create_provider idiom in composition). Proven end-to-end at the provider level against a real local mem0 stack (store -> search-recall -> verbatim read). It does not fully close #3537: native SQL storage-port backing (reborn_memory_* tables), the host-managed memory.context.retrieve / memory.interaction.record flow, the default flip + /memory migration, and memory.semantic_search.v1 remain follow-ups tracked in #5264.

Memory is now a host-bundled userland extension that implements host-defined capability profiles, bound per profile through a fail-closed profile_id → extension_id policy — consolidated to a single, always-on native surface.

End state: one always-on native memory extension

ironclaw.memory.native is a bundled v2 Extension Manifest (schema_version = "reborn.extension_manifest.v2") parsed from crates/ironclaw_host_runtime/assets/memory_native/manifest.toml and registered on the always-on first-party lane (the same lane as the builtin toolset) — not the catalog/lifecycle lane — so its tools are unconditionally available with no install/enable step. It declares four model-visible memory tools (read/write/search/tree); read/write implements memory.document_store.v1 (their schema refs match the profile's operation refs), and search/tree are native conveniences that implement no profile. The model-facing tool names are ironclaw__memory__native__{read,write,search,tree}.

This replaces the prior two declarations of the one filesystem-backed provider — the live builtin.memory_* tools and a separate, dormant host_internal native manifest — with one. builtin.memory_* is removed; provider-swapping is governed by the document-store profile binding (config-driven), not by install/enable.

The live provider is filesystem-backed and declares no host ports; input schemas are served inline (include_str! of the bundled asset files, the single source of truth) on the always-on lane rather than materialized. Behavior, I/O, the scoped /memory mount, and on-by-default availability are unchanged from builtin.memory_*; only the capability identity + derived tool names change.

Kept #3537 machinery

  • Profile catalog + conformance (ironclaw_host_runtime::memory_profiles): the three host-defined contracts (memory.context_retrieval.v1, memory.interaction_log.v1, memory.document_store.v1) and the semantic conformance harness. The native read/write capabilities are proven to satisfy memory.document_store.v1.
  • Host-port catalog (default_host_port_catalog()): host.storage.sql_transaction.first_party + host.events.audit stay catalogued as deferred-future vocabulary.
  • Profile binding (MemoryBindingPolicy, fail-closed; the [memory] config section; MemoryServiceResolver single construction point): default-native, production rejects memory.disabled and unverified third-party bindings absent an (extension_id, profile_id, deployment_profile) admin override.
  • The feat(memory): model memory as a userland extension (#3537) #5163 follow-ups: host-owned context sanitization, the workspace-allowlist boundary test, profile reads through the MemoryService facade.

Deferred (not stubbed)

  • Host-managed context/interaction flowcontext_retrieval / interaction_log are defined and conformance-testable but have no live implementer; wiring the host turn pipeline to invoke memory.context.retrieve before model calls and record sanitized interactions via memory.interaction.record afterward is deferred. The live native surface ships only the model-facing document-store tools.
  • SQL storage-port-backed native persistence — the reborn_memory_* dual-backend tables behind host.storage.sql_transaction.first_party, behind non-default memory-native-* features (the reborn-composition crates cannot depend on the root ironclaw crate where the Postgres/libSQL backends live). See docs/adr/0002-native-memory-uses-host-storage-ports.md.
  • Default flip + memory.semantic_search.v1 — unchanged from the issue's deferral (/memory data/API compatibility decision; host-mediated embedding port).

Gates

  • cargo fmt --all -- --check — clean
  • cargo clippy -p ironclaw_host_runtime -p ironclaw_reborn_composition --tests --features "root-llm-provider webui-v2-beta libsql" — zero warnings
  • cargo test -p ironclaw_host_runtime — green (the 3 sandbox_process failures are pre-existing, Docker-only)
  • cargo test -p ironclaw_reborn_composition --features "root-llm-provider webui-v2-beta libsql" — green (the 1 skill_learned_bubble failure is pre-existing under this feature combo on main)
  • cargo test -p ironclaw_architecture — green (no new cross-crate dependency edges)
  • python3 scripts/check_no_panics.py --base main --head HEAD — OK

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: eb89d5d6-a99f-4bbd-ab5d-39845424b2c7

📥 Commits

Reviewing files that changed from the base of the PR and between f8377b9 and 502f676.

📒 Files selected for processing (2)
  • crates/ironclaw_memory_mem0/src/service.rs
  • crates/ironclaw_reborn_composition/src/factory.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/ironclaw_memory_mem0/src/service.rs
  • crates/ironclaw_reborn_composition/src/factory.rs

📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Added bundled ironclaw.memory.native first-party extension with new read/write/search/tree prompts and strict JSON schemas.
    • Introduced fail-closed, host-owned memory document-store binding with resolver-driven selection and feature-gated mem0 provider support.
    • Added default host ports for SQL first-party transactions and durable audit events.
  • Bug Fixes
    • Hardened memory context snippet handling with deterministic references, stricter sanitization, and tighter per-snippet/aggregate budgeting.
  • Tests
    • Expanded boundary enforcement, schema/conformance, prompt/context regression, and mem0 swap/E2E coverage.
  • Documentation
    • Updated ADRs and memory contract docs to reflect native port-bound manifests and new binding semantics.

Walkthrough

Adds host memory profiles, native and mem0 memory provider wiring, host-owned snippet/profile reads, Reborn startup/config plumbing, and allowlist-based dependency-boundary checks.

Changes

Memory rollout

Layer / File(s) Summary
Host memory contracts and native package
crates/ironclaw_host_api/src/host_port.rs, crates/ironclaw_host_runtime/src/extension_contracts.rs, crates/ironclaw_host_runtime/src/memory_profiles.rs, crates/ironclaw_host_runtime/src/memory_native_extension.rs, crates/ironclaw_host_runtime/assets/memory_native/*, crates/ironclaw_host_runtime/src/surface.rs, crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs, crates/ironclaw_host_runtime/src/lib.rs, crates/ironclaw_host_runtime/tests/memory_native_*, crates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs, tests/reborn_trace_first_party_tool_coverage.rs
Host port IDs, memory profile contracts, the bundled ironclaw.memory.native manifest/package, native schemas/prompts, schema resolution, and conformance coverage are added.
Host-owned snippet and profile reads
crates/ironclaw_memory/src/service.rs, crates/ironclaw_memory_native/src/service.rs, crates/ironclaw_host_runtime/src/memory_context.rs, crates/ironclaw_host_runtime/tests/memory_prompt_context.rs, crates/ironclaw_memory_native/tests/memory_service_facade.rs, crates/ironclaw_host_runtime/src/user_profile_source.rs, crates/ironclaw_host_runtime/tests/user_profile_roundtrip.rs
MemoryService adds profile_read, the native provider returns raw snippets and profile bytes, the host derives refs and safety wrappers, and user-profile reads switch to the memory service.
Memory binding and dispatch
crates/ironclaw_host_runtime/src/memory_binding.rs, crates/ironclaw_host_runtime/src/memory_provider.rs, crates/ironclaw_host_runtime/src/first_party_tools/memory.rs, crates/ironclaw_host_runtime/src/first_party_tools/mod.rs, crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs, crates/ironclaw_host_runtime/src/lib.rs, crates/ironclaw_host_runtime/src/surface.rs, crates/ironclaw_reborn_composition/src/projection/display_preview.rs, crates/ironclaw_reborn_composition/src/projection/tests/display_preview.rs, crates/ironclaw_reborn_composition/src/runtime.rs, crates/ironclaw_reborn_composition/src/runtime/local_dev.rs, crates/ironclaw_reborn_composition/src/input.rs, crates/ironclaw_reborn_composition/src/lib.rs, crates/ironclaw_reborn_composition/src/memory_binding.rs, crates/ironclaw_reborn_composition/src/memory_provider_factory.rs, crates/ironclaw_reborn_composition/tests/memory_mem0_swap.rs, crates/ironclaw_reborn_cli/src/runtime/mod.rs, crates/ironclaw_reborn_config/src/config_file.rs, tests/support/reborn/harness.rs, crates/ironclaw_reborn_composition/src/factory.rs
MemoryBindingPolicy resolves native, disabled, and third-party bindings, and MemoryServiceResolver builds or reuses the selected document-store provider.
Reborn config and provider assembly
Cargo.toml, crates/ironclaw_reborn_cli/Cargo.toml, crates/ironclaw_reborn_composition/Cargo.toml, crates/ironclaw_reborn_composition/src/{local_dev_capability_policy.toml,projection/display_preview.rs,lib.rs, input.rs, memory_binding.rs, memory_provider_factory.rs, factory.rs, runtime.rs, runtime/local_dev.rs}, crates/ironclaw_reborn_config/src/{config_file.rs,lib.rs}, tests/support/reborn/harness.rs, crates/ironclaw_reborn_composition/tests/memory_mem0_swap.rs, scripts/ci/package-feature-flags.sh, docs/adr/*, docs/reborn/contracts/memory-profiles.md
The mem0 adapter crate adds configuration, transport, URL validation, service mapping, and composition wiring for the third-party memory lane, while Reborn config, CLI startup, runtime assembly, docs, tests, and harness wiring thread the resolved memory binding through startup.
Mem0 provider and transport
crates/ironclaw_memory_mem0/Cargo.toml, crates/ironclaw_memory_mem0/src/{config.rs,error.rs,lib.rs,service.rs,transport.rs,url_check.rs}, crates/ironclaw_memory_mem0/tests/live_local_mem0.rs
The mem0 adapter crate adds configuration, transport, URL validation, service mapping, and live and unit coverage for the third-party memory lane.

Possibly related issues

Possibly related PRs

  • nearai/ironclaw#5008 — This PR reroutes user_profile_source.rs through MemoryService::profile_read, continuing the profile persistence flow from that PR.
  • nearai/ironclaw#5163 — Both PRs route host-runtime memory tools through the provider-neutral ironclaw_memory::MemoryService facade.
  • nearai/ironclaw#5110 — Both PRs touch the same CI feature-flag plumbing in scripts/ci/package-feature-flags.sh.

Suggested labels
size: XL, risk: medium, scope: docs, scope: dependencies, contributor: experienced

Suggested reviewers

  • henrypark133
  • serrrfirat

Poem

A native port woke up in tune,
With profiles, prompts, and mem0 soon.
The host now trims each snippet clean,
And routes each binding through the seam. 🌿

Estimated code review effort
🎯 5 (Critical) | ⏱️ ~120 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: modeling memory as a userland extension, and it matches the changeset.
Description check ✅ Passed The description covers the main summary, issue linkage, validation, and trust/security details, though it doesn't follow the template headings exactly.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch reborn/memory-lift-followups

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size: XL 500+ changed lines risk: low Changes to docs, tests, or low-risk modules contributor: regular 2-5 merged PRs labels Jun 25, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the memory context and user profile loading architecture, shifting the responsibility of reference hashing, sanitization, untrusted-envelope wrapping, and model-visible budget enforcement from the native memory provider to the host runtime. It also introduces a provider-neutral profile_read method to the MemoryService trait and updates workspace dependency boundary tests to use allowlists. Feedback suggests optimizing the snippet sanitization logic by caching the untrusted envelope prefix length using OnceLock to avoid redundant allocations on every snippet.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread crates/ironclaw_host_runtime/src/memory_context.rs
Base automatically changed from reborn/memory-placement-m2-lift to main June 25, 2026 01:57
@BenKurrek BenKurrek marked this pull request as draft June 25, 2026 02:58
@BenKurrek BenKurrek changed the title feat(memory): host-owned context sanitization + boundary allowlist + profile read facade (#5163 follow-ups) feat(memory): model memory as a userland extension — implements #3537 Jun 25, 2026
@github-actions github-actions Bot added scope: docs Documentation contributor: experienced 6-19 merged PRs and removed contributor: regular 2-5 merged PRs labels Jun 25, 2026
@BenKurrek BenKurrek marked this pull request as ready for review June 25, 2026 13:41

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs (1)

1712-1727: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Stale "forbidden deps" wording after the denylist→allowlist switch.

Line 1715 still reads "Adding any of the forbidden deps here…", but rules no longer carry a forbidden list — anything not in allowed is now rejected. Reword to the allowlist semantics so the contract comment matches the enforcement.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs` around
lines 1712 - 1727, The contract comment in the `BoundaryRule` for
`ironclaw_reborn_cli` still describes the rule as a forbidden-deps list, but the
enforcement now uses an allowlist. Update the wording near the `allowed` field
so it clearly says anything not in `allowed` is rejected, while keeping the same
intent about protecting `ironclaw_reborn_composition`-mediated access to
internal Reborn types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json`:
- Around line 7-10: Tighten the document-read.input.v1.json schema so the path
field is restricted to scoped relative paths only, not just any non-empty
string. Update the schema around the path property to enforce a relative,
non-absolute form and reject traversal patterns like leading slashes or
parent-directory segments, keeping the change localized to the document-read
input schema definition.

In `@crates/ironclaw_host_runtime/src/memory_binding.rs`:
- Around line 343-349: The override lookup in `memory_binding.rs` is comparing
`extension_id` with raw string equality, so case-only differences like
`acme.honcho` vs `Acme.Honcho` are missed. Update the `has_override` check
inside `requires_certified_bindings()` to normalize both sides to lowercase
using `.to_ascii_lowercase()` before comparing extension names, while keeping
the existing profile and deployment matching logic unchanged.
- Around line 138-144: The redaction logic in redacted_summary uses byte slicing
on extension_id, which can panic for non-ASCII input. Update redacted_summary to
truncate safely by characters/graphemes instead of &ext[..12], preserving the
existing redacted_ext behavior while avoiding any direct byte-index slicing on
external strings.

In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 88-96: The host-runtime memory admission path in
`admit_memory_context_snippet` / `admit_memory_context_snippets` currently
trusts provider-returned scope too early, so revalidate each snippet against
`request.scope` and `request.actor` before calling
`admit_memory_context_snippet` or hashing/wrapping it. Add a scope check in the
snippet loop to reject any tenant/user/agent/project mismatch, then only admit
snippets that match the request’s scope and actor. Keep the existing byte limit
and admission flow unchanged for valid snippets.

In `@crates/ironclaw_host_runtime/tests/memory_native_schema_validation.rs`:
- Around line 52-68: The current test only validates
`context_retrieve_input_schema_accepts_valid_and_rejects_invalid`, so add
instance-level valid/invalid assertions for the new document-store and
interaction schemas as well. Use the existing `load_schema` and
`jsonschema::validator_for` pattern in this test module to cover
`document-read`, `document-write`, and `interaction-record`, including checks
for required fields, enum values, numeric bounds, and rejection of unexpected
properties. Keep the fixtures alongside the existing schema validation tests so
regressions in those contracts fail in the same suite.

In `@crates/ironclaw_memory/src/service.rs`:
- Around line 345-353: `MemoryServiceContextSnippet` is carrying identity
components as raw strings across the service boundary, so replace those fields
with the validated identifier newtypes used by the domain (for example
`TenantId`, `UserId`, `AgentId`, `ProjectId`) or add a typed wrapper before
hashing in `MemoryServiceContextSnippet`/`memory_context.rs`. Update any
constructors, serialization, and call sites so the context can only be built
from validated IDs, preserving the strong contract and preventing malformed or
swapped scope components from changing the snippet reference.

In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 2914-2921: The document_store_binding() helper is silently falling
back to MemoryProviderBinding::Native when a supplied MemoryBindingPolicy cannot
resolve the document-store capability or has no matching binding. Change
document_store_binding(policy: Option<&MemoryBindingPolicy>) to return
Result<MemoryProviderBinding, RebornBuildError>, propagate
CapabilityProfileId::new(MEMORY_DOCUMENT_STORE_PROFILE_ID)? instead of dropping
the error, and make the Some(policy) / missing-binding path fail closed with an
error rather than defaulting to Native. Keep only the None policy case as the
native default, and update the call sites to use ? where document_store_binding
is consumed.

In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 3037-3042: The profile source selection in runtime.rs is still
hardwired to the native filesystem path inside the user_profile_source branch,
which bypasses the resolved memory binding. Update the logic around
local_runtime and MemoryBackedUserProfileSourceAdapter to use the same
binding-aware Arc<dyn MemoryService> that memory capabilities use, constructing
MemoryBackedUserProfileSource with that service instead of from_filesystem; if
no valid binding is available, fall back to EmptyUserProfileSource so profile
reads fail closed rather than opening context/profile.json.

---

Outside diff comments:
In `@crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs`:
- Around line 1712-1727: The contract comment in the `BoundaryRule` for
`ironclaw_reborn_cli` still describes the rule as a forbidden-deps list, but the
enforcement now uses an allowlist. Update the wording near the `allowed` field
so it clearly says anything not in `allowed` is rejected, while keeping the same
intent about protecting `ironclaw_reborn_composition`-mediated access to
internal Reborn types.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c15673fc-f28f-4539-939f-0ac535fa233b

📥 Commits

Reviewing files that changed from the base of the PR and between c02f73d and 4a53d9b.

📒 Files selected for processing (40)
  • crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs
  • crates/ironclaw_host_api/src/host_port.rs
  • crates/ironclaw_host_runtime/assets/memory_native/manifest.toml
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/context-retrieve.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/context-retrieve.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/interaction-record.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/interaction-record.output.v1.json
  • crates/ironclaw_host_runtime/src/extension_contracts.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/memory.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/mod.rs
  • crates/ironclaw_host_runtime/src/lib.rs
  • crates/ironclaw_host_runtime/src/memory_binding.rs
  • crates/ironclaw_host_runtime/src/memory_context.rs
  • crates/ironclaw_host_runtime/src/memory_native_extension.rs
  • crates/ironclaw_host_runtime/src/memory_profiles.rs
  • crates/ironclaw_host_runtime/src/user_profile_source.rs
  • crates/ironclaw_host_runtime/tests/memory_capability_profiles.rs
  • crates/ironclaw_host_runtime/tests/memory_native_manifest.rs
  • crates/ironclaw_host_runtime/tests/memory_native_schema_validation.rs
  • crates/ironclaw_host_runtime/tests/memory_prompt_context.rs
  • crates/ironclaw_host_runtime/tests/user_profile_roundtrip.rs
  • crates/ironclaw_memory/src/lib.rs
  • crates/ironclaw_memory/src/service.rs
  • crates/ironclaw_memory_native/src/service.rs
  • crates/ironclaw_memory_native/tests/memory_service_facade.rs
  • crates/ironclaw_reborn_cli/src/runtime/mod.rs
  • crates/ironclaw_reborn_composition/src/factory.rs
  • crates/ironclaw_reborn_composition/src/input.rs
  • crates/ironclaw_reborn_composition/src/lib.rs
  • crates/ironclaw_reborn_composition/src/memory_binding.rs
  • crates/ironclaw_reborn_composition/src/runtime.rs
  • crates/ironclaw_reborn_config/src/config_file.rs
  • crates/ironclaw_reborn_config/src/lib.rs
  • docs/adr/0001-capability-manifest-v2-hard-cutover.md
  • docs/adr/0002-native-memory-uses-host-storage-ports.md
  • docs/reborn/contracts/memory-profiles.md

Comment thread crates/ironclaw_host_runtime/src/memory_binding.rs
Comment thread crates/ironclaw_host_runtime/src/memory_binding.rs
Comment thread crates/ironclaw_host_runtime/src/memory_context.rs
Comment thread crates/ironclaw_memory/src/service.rs
Comment thread crates/ironclaw_reborn_composition/src/factory.rs Outdated
Comment thread crates/ironclaw_reborn_composition/src/runtime.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/runtime.rs (1)

3018-3042: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the stale workspace-filesystem contract comment.

Lines 3018-3021 still say profile reads go through the workspace filesystem, but the code now gates on memory_document_store_binding and builds from extension_filesystem. Soften or rewrite that header comment to match the new binding-aware path. As per coding guidelines, comments that promise cross-layer guarantees must match enforced behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 3018 - 3042,
The leading workspace-filesystem contract comment is stale and no longer matches
the binding-aware implementation. Update the header comment near the
profile-source setup in runtime.rs so it describes the current
`memory_document_store_binding`-gated path and the use of
`extension_filesystem`, and remove or soften any claim that profile reads always
go through the workspace filesystem. Keep the surrounding references to
`MemoryBackedUserProfileSource`, `EmptyUserProfileSource`, and
`identity_context_source` aligned with the actual control flow.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_host_runtime/src/memory_context.rs`:
- Around line 420-464: Add a caller-level regression in
ProductionMemoryPromptContextService::load_memory_snippets to verify that
snippets with the correct tenant/user but mismatched agent_id or project_id are
filtered out before being returned. Keep the existing
admit_memory_context_snippet helper tests, but add an integration-style test in
the memory_prompt_context test suite that exercises the provider path and
asserts the bad snippet is dropped while matching scope data still passes
through.

In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 2928-2948: The missing document-store binding case is only covered
at the helper level, so add a factory-level regression in the build path around
build_reborn_services in factory.rs. Exercise a resolved MemoryBindingPolicy
that omits memory.document_store.v1, then assert the build fails with
RebornBuildError::InvalidConfig from document_store_binding. Use the existing
factory/build symbols to keep the test anchored to the caller and ensure the
invariant is validated end-to-end.

---

Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 3018-3042: The leading workspace-filesystem contract comment is
stale and no longer matches the binding-aware implementation. Update the header
comment near the profile-source setup in runtime.rs so it describes the current
`memory_document_store_binding`-gated path and the use of
`extension_filesystem`, and remove or soften any claim that profile reads always
go through the workspace filesystem. Keep the surrounding references to
`MemoryBackedUserProfileSource`, `EmptyUserProfileSource`, and
`identity_context_source` aligned with the actual control flow.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: be9e9128-9127-4643-a9e3-0a847a478f4b

📥 Commits

Reviewing files that changed from the base of the PR and between 4a53d9b and 5c79205.

📒 Files selected for processing (5)
  • crates/ironclaw_host_runtime/src/memory_binding.rs
  • crates/ironclaw_host_runtime/src/memory_context.rs
  • crates/ironclaw_host_runtime/tests/memory_native_schema_validation.rs
  • crates/ironclaw_reborn_composition/src/factory.rs
  • crates/ironclaw_reborn_composition/src/runtime.rs

Comment thread crates/ironclaw_host_runtime/src/memory_context.rs
Comment thread crates/ironclaw_reborn_composition/src/factory.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/runtime.rs (1)

3018-3021: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the stale profile-source comment.

This still says profile context comes from context/profile.json via the workspace filesystem, but the changed code resolves through MemoryServiceResolver and MemoryService::profile_read. Soften or rewrite these lines to avoid documenting the old provider/path contract.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 3018 - 3021,
The comment above the profile resolution flow is stale and still documents the
old `context/profile.json` workspace-filesystem contract; update it to match the
current `MemoryServiceResolver` and `MemoryService::profile_read` path. Rewrite
the text near the profile-source logic so it describes the current in-memory
service-based resolution and the graceful `None` fallback without mentioning the
deprecated provider or file path.

Sources: Coding guidelines, Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 3018-3021: The comment above the profile resolution flow is stale
and still documents the old `context/profile.json` workspace-filesystem
contract; update it to match the current `MemoryServiceResolver` and
`MemoryService::profile_read` path. Rewrite the text near the profile-source
logic so it describes the current in-memory service-based resolution and the
graceful `None` fallback without mentioning the deprecated provider or file
path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e9a1809b-6f40-4c01-aa32-7077341184c9

📥 Commits

Reviewing files that changed from the base of the PR and between 5c79205 and 70ba106.

📒 Files selected for processing (6)
  • crates/ironclaw_host_runtime/src/first_party_tools/memory.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/mod.rs
  • crates/ironclaw_host_runtime/src/lib.rs
  • crates/ironclaw_host_runtime/src/memory_provider.rs
  • crates/ironclaw_reborn_composition/src/factory.rs
  • crates/ironclaw_reborn_composition/src/runtime.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

♻️ Duplicate comments (1)
crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json (1)

4-7: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Constrain path to scoped relative form.

The schema still accepts absolute and ../ traversal paths, so the read tool does not enforce the relative-path contract documented in read.md and the manifest. Add the same pattern guard here.

Based on the manifest wiring and the previous schema review note.

🐛 Suggested schema hardening
     "path": {
       "type": "string",
       "description": "Relative memory document path to read"
+      ,
+      "pattern": "^(?!/)(?!.*(?:^|/)\\.\\.(?:/|$))(?!.*\\\\)[A-Za-z0-9._/-]+$"
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json`
around lines 4 - 7, The document read input schema still allows unsafe path
values; tighten the `path` field in the `memory/document-read.input.v1.json`
schema to match the scoped relative-path contract. Update the `path` definition
used by the read tool so it rejects absolute paths and `../` traversal,
mirroring the same pattern guard used in the manifest and other memory schemas.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_host_runtime/assets/memory_native/manifest.toml`:
- Line 20: The manifest has duplicated capability table headers in the
memory_native bundle, which creates empty capability entries before the real
ones. Remove the extra repeated [[capabilities]] headers so each capability
block in manifest.toml maps to only one table, and verify the populated entries
remain under the existing capability sections.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.json`:
- Around line 4-40: The document-write schema currently allows invalid mixed or
incomplete requests because it only uses a strict object shape without enforcing
write-mode invariants. Update the schema in document-write.input.v1.json to add
explicit required fields and conditional branches (for plain writes, patch-mode
writes using old_string/new_string/replace_all, and the bootstrap clear path) so
the accepted shapes are mutually exclusive and invalid payloads like empty
objects or mixed modes are rejected at the schema boundary. Use the existing
target, content, append, old_string, new_string, replace_all, and bootstrap
fields as the anchors for these rules.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json`:
- Around line 4-16: The document-write output schema does not actually
discriminate fields by status, so outcome-specific properties can appear or be
missing incorrectly. Update the schema in document-write.output.v1.json to use
status-based validation with oneOf or if/then/else so cleared, patched, and
written each require only their own fields and forbid the others. Keep the
existing status, path, and additionalProperties constraints, and make the rules
explicit around the status enum plus the message, replacements, and append
properties.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.json`:
- Around line 4-29: The memory search schema currently advertises alias fields
in search.input.v1.json, but the required constraint still forces query, so
q/text/pattern payloads fail validation. Update the schema so the search
contract accepts at least one of the canonical query inputs (for example via
oneOf/anyOf around query, q, text, and pattern) and clearly define how conflicts
are handled, or remove the alias fields entirely if they are not supported.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.output.v1.json`:
- Around line 10-17: The search output schema still allows free-form values for
path, which can expose absolute or parent-relative paths to the model. Tighten
the memory/search.output.v1.json schema so the path field uses the same
scoped-relative restriction as the read schema, and keep the existing search
result shape in sync by updating the path property definition in the search
result schema.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.json`:
- Around line 2-4: The tree schema currently uses the untyped items field in
memory/tree.output.v1.json, which removes validation for array elements
entirely. Update the schema so the array items are constrained to the actual
node shape used by the memory tree, ideally with a typed object or union that
distinguishes document vs directory entries, and keep the schema aligned with
the runtime types referenced by the tree output contract.

In `@crates/ironclaw_reborn_composition/src/local_dev_capability_policy.toml`:
- Around line 117-126: Remove the default network grant from the live
ironclaw.memory.native capability entries in local_dev_capability_policy.toml so
the native memory search/write grants match the filesystem-only trust model.
Update the relevant grants for ironclaw.memory.native.search and
ironclaw.memory.native.write to keep only the dispatch/filesystem authority
already reflected in factory.rs and runtime/local_dev.rs, and ensure no network
= "default" remains for these always-on memory tools.

---

Duplicate comments:
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json`:
- Around line 4-7: The document read input schema still allows unsafe path
values; tighten the `path` field in the `memory/document-read.input.v1.json`
schema to match the scoped relative-path contract. Update the `path` definition
used by the read tool so it rejects absolute paths and `../` traversal,
mirroring the same pattern guard used in the manifest and other memory schemas.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a99c0ebd-432e-42a6-8503-2dd5dab92f8c

📥 Commits

Reviewing files that changed from the base of the PR and between 70ba106 and fa0b1b9.

📒 Files selected for processing (25)
  • crates/ironclaw_host_runtime/assets/memory_native/manifest.toml
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/read.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/search.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/tree.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/write.md
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.json
  • crates/ironclaw_host_runtime/src/first_party_tools/memory.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/mod.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs
  • crates/ironclaw_host_runtime/src/lib.rs
  • crates/ironclaw_host_runtime/src/memory_native_extension.rs
  • crates/ironclaw_host_runtime/src/surface.rs
  • crates/ironclaw_host_runtime/tests/memory_native_manifest.rs
  • crates/ironclaw_reborn_composition/src/factory.rs
  • crates/ironclaw_reborn_composition/src/local_dev_capability_policy.toml
  • crates/ironclaw_reborn_composition/src/runtime/local_dev.rs
  • docs/adr/0002-native-memory-uses-host-storage-ports.md
  • docs/reborn/contracts/memory-profiles.md

Comment thread crates/ironclaw_host_runtime/assets/memory_native/manifest.toml

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_memory/src/service.rs`:
- Around line 74-84: The `target` fallback in `service.rs` is currently
unreachable on the schema-validated path because `document-write.input.v1.json`
still only allows a string. Update the schema and the
`parse_write_command`/input parsing logic together so `target: null` is either
accepted intentionally or removed from the compatibility handling, and then add
a regression test covering the intended `target` behavior through the
schema-driven flow.

In `@tests/support/reborn/harness.rs`:
- Around line 2078-2094: The native-memory trust tuple added for
core_builtin_tools_from_runtime is still missing from
HostRuntimeCapabilityHarness::qa_smoke_tools(), where the MEMORY_* capability
IDs are still wired to builtin with no additional_provider_trust. Extract the
new trust tuple into a shared helper and reuse it in both harness constructors
so qa_smoke_tools() also trusts
ExtensionId::new(NATIVE_MEMORY_FIRST_PARTY_PROVIDER) with the same
DispatchCapability, ReadFilesystem, and WriteFilesystem effects. This keeps
create_capability_port() authorization aligned with the split provider model and
prevents QA-smoke memory grants from failing closed as untrusted.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8ab5cbdb-b69d-41e2-a595-8e13de85095f

📥 Commits

Reviewing files that changed from the base of the PR and between dc2fbb5 and 976a171.

📒 Files selected for processing (7)
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.json
  • crates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs
  • crates/ironclaw_host_runtime/tests/memory_native_schema_validation.rs
  • crates/ironclaw_memory/src/service.rs
  • tests/reborn_trace_first_party_tool_coverage.rs
  • tests/support/reborn/harness.rs

Comment thread crates/ironclaw_memory/src/service.rs
Comment thread tests/support/reborn/harness.rs
@github-actions github-actions Bot added scope: dependencies Dependency updates risk: medium Business logic, config, or moderate-risk modules and removed risk: low Changes to docs, tests, or low-risk modules labels Jun 25, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs`:
- Around line 1599-1611: The `ironclaw_memory_mem0` boundary entry only enforces
its allowed dependencies and does not actually prevent `ironclaw_host_runtime`
from naming this crate. Add an explicit boundary rule for
`ironclaw_host_runtime` in `reborn_dependency_boundaries.rs` that excludes
`ironclaw_memory_mem0`, or revise the nearby comment to describe the restriction
as intended rather than enforced. Use the existing `BoundaryRule` entries in the
test file to keep the new rule consistent with the other crate boundary
definitions.

In `@crates/ironclaw_memory_mem0/src/service.rs`:
- Around line 186-205: The mem0 read and profile paths only consume the first
list page, so results beyond page 1 are missed. Update the list-fetch logic in
the relevant service methods in service.rs to keep following mem0 pagination
using the page/next fields until all memories are collected, then rebuild the
document/profile from the full set. Apply this to the code paths that perform
the list call and reconstruct results so reads and profile_read do not stop at
the first page.

In `@crates/ironclaw_memory_mem0/src/transport.rs`:
- Around line 201-205: The mem0 body parsing in the transport boundary currently
hides malformed JSON by using `serde_json::from_str(&text).unwrap_or_default()`,
which silently swallows parse errors. Update the body handling in `transport.rs`
to either propagate the `serde_json::from_str` failure from this external IO
boundary or replace it with an explicitly documented tolerated fallback marked
with a `silent-ok` rationale and a log that records only the parse error; keep
the `body` construction in the same `let body = if text.trim().is_empty()` flow.
- Around line 157-160: The reqwest client built in Mem0HttpTransport::new is
missing a timeout, so add a bounded client timeout directly in the
reqwest::Client::builder chain before build() and keep the existing error
mapping intact. Use the existing client construction path in transport.rs to
ensure all mem0 HTTP requests inherit the limit rather than relying on upstream
callers.

In `@crates/ironclaw_memory_mem0/src/url_check.rs`:
- Around line 28-57: Reject non-path URL components in check_base_url: the
current reqwest::Url validation in url_check.rs still allows credentials, query
strings, and fragments in Mem0 base URLs. Update check_base_url to fail closed
when parsed.username()/parsed.password() are present, or when parsed.query() or
parsed.fragment() is set, and return Mem0Error::InvalidUrl with a clear reason.
Keep the existing scheme/host/IP checks intact while enforcing this stricter
boundary on base URLs.

In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 3284-3287: The startup path is currently failing open when the
selected memory provider cannot be constructed, so update the
`build_memory_service_resolver()` flow used in `factory.rs` to fail closed
instead of returning a resolver without the third-party provider. Make the
resolver construction or the surrounding production validation return a
`Result`, and map missing or rejected provider handles from
`MemoryProviderDeps::for_third_party(...)` to `RebornBuildError::InvalidConfig`
before reporting ready. Apply the same fix to both call sites that build
`memory_resolver`, so production and migration-dry-run profiles refuse startup
when the selected memory provider is unavailable.

In `@crates/ironclaw_reborn_composition/src/lib.rs`:
- Around line 231-234: The crate root is re-exporting internal provider factory
seams instead of only facade-shaped composition APIs. Remove the public exposure
of MemoryProviderDeps, Mem0ConnectionConfig, and create_document_store_provider
from the lib.rs re-export, and keep them private or pub(crate) within
memory_provider_factory; if tests need access, gate that access behind
test-support rather than the production crate root. Keep
build_memory_service_resolver as the only surfaced composition entrypoint if it
is the intended facade.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 76613def-41db-434d-8a35-094f1930e00d

📥 Commits

Reviewing files that changed from the base of the PR and between 976a171 and ec28a9d.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock
📒 Files selected for processing (20)
  • Cargo.toml
  • crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/memory.rs
  • crates/ironclaw_host_runtime/src/memory_provider.rs
  • crates/ironclaw_memory_mem0/Cargo.toml
  • crates/ironclaw_memory_mem0/src/config.rs
  • crates/ironclaw_memory_mem0/src/error.rs
  • crates/ironclaw_memory_mem0/src/lib.rs
  • crates/ironclaw_memory_mem0/src/service.rs
  • crates/ironclaw_memory_mem0/src/transport.rs
  • crates/ironclaw_memory_mem0/src/url_check.rs
  • crates/ironclaw_reborn_cli/src/runtime/mod.rs
  • crates/ironclaw_reborn_composition/Cargo.toml
  • crates/ironclaw_reborn_composition/src/factory.rs
  • crates/ironclaw_reborn_composition/src/input.rs
  • crates/ironclaw_reborn_composition/src/lib.rs
  • crates/ironclaw_reborn_composition/src/memory_binding.rs
  • crates/ironclaw_reborn_composition/src/memory_provider_factory.rs
  • crates/ironclaw_reborn_composition/tests/memory_mem0_swap.rs
  • crates/ironclaw_reborn_config/src/config_file.rs

Comment thread crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs
Comment thread crates/ironclaw_memory_mem0/src/service.rs Outdated
Comment thread crates/ironclaw_memory_mem0/src/transport.rs
Comment thread crates/ironclaw_memory_mem0/src/transport.rs
Comment thread crates/ironclaw_reborn_composition/src/factory.rs
Comment thread crates/ironclaw_reborn_composition/src/lib.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/ironclaw_memory_mem0/src/service.rs (1)

236-246: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Sort document parts before concatenating mem0 reads.

Line 246 joins matched memories in backend list order, but Line 454 notes mem0/Qdrant listing is not chronological. Multiple writes to the same target can therefore read back in nondeterministic order.

Suggested fix
-        let parts: Vec<String> = response_items(&response.body)
+        let mut parts: Vec<(String, usize, String)> = response_items(&response.body)
             .into_iter()
+            .enumerate()
-            .filter(|item| item_metadata_str(item, TARGET_KEY) == Some(request.path.as_str()))
-            .filter_map(|item| item_text(item).map(str::to_string))
+            .filter(|(_, item)| item_metadata_str(item, TARGET_KEY) == Some(request.path.as_str()))
+            .filter_map(|(index, item)| {
+                item_text(item)
+                    .map(|text| (item_created_at(item).to_string(), index, text.to_string()))
+            })
             .collect();
         if parts.is_empty() {
             return Err(MemoryServiceError::input());
         }
-        let content = parts.join("\n");
+        parts.sort_by(|(left_created_at, left_index, _), (right_created_at, right_index, _)| {
+            left_created_at
+                .cmp(right_created_at)
+                .then(left_index.cmp(right_index))
+        });
+        let content = parts
+            .into_iter()
+            .map(|(_, _, part)| part)
+            .collect::<Vec<_>>()
+            .join("\n");

Also applies to: 454-456

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_memory_mem0/src/service.rs` around lines 236 - 246, The
reconstructed document in the mem0 read path is joined in backend list order,
but that order is not guaranteed to be chronological. Update the logic in the
response processing block that builds `parts` from
`response_items(&response.body)` so the matched memories are sorted
deterministically before `join("\n")`, using the available metadata/timestamp
fields from the item payload. Apply the same ordering assumption consistently
with the note in the later mem0/Qdrant listing logic so repeated writes to the
same `target` read back in a stable order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_memory_mem0/src/service.rs`:
- Around line 122-142: The fallback in fetch_latest_profile_object is silently
discarding malformed profile JSON and turning it into an empty object, which can
cause profile_set to overwrite existing fields. Update this method to
distinguish “no prior profile” from “parse failed” by propagating serde_json
parse errors as MemoryServiceError instead of using unwrap_or_default. Keep the
empty-object fallback only for genuinely missing profile data from
latest_profile_text/response_items, and ensure any intentional silent fallback
is explicitly justified with a silent-ok comment if retained.

In `@crates/ironclaw_reborn_config/src/config_file.rs`:
- Around line 122-131: Validate memory.mem0_base_url alongside the other
operator-supplied string fields in the config validation path, not just bindings
and overrides. Update the validation logic in config_file.rs so the
mem0_base_url value is trimmed and checked for inline-secret/private-URL
patterns the same way the existing operator-supplied strings are handled, and
reject whitespace-only or credential-bearing URLs before provider startup. Use
the existing config validation helpers and the mem0_base_url field on the config
struct as the main points to update.

---

Outside diff comments:
In `@crates/ironclaw_memory_mem0/src/service.rs`:
- Around line 236-246: The reconstructed document in the mem0 read path is
joined in backend list order, but that order is not guaranteed to be
chronological. Update the logic in the response processing block that builds
`parts` from `response_items(&response.body)` so the matched memories are sorted
deterministically before `join("\n")`, using the available metadata/timestamp
fields from the item payload. Apply the same ordering assumption consistently
with the note in the later mem0/Qdrant listing logic so repeated writes to the
same `target` read back in a stable order.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a49e4842-2956-4e21-a069-008dc1fef1b0

📥 Commits

Reviewing files that changed from the base of the PR and between 213b068 and a16aa72.

📒 Files selected for processing (8)
  • crates/ironclaw_memory_mem0/src/lib.rs
  • crates/ironclaw_memory_mem0/src/service.rs
  • crates/ironclaw_memory_mem0/src/transport.rs
  • crates/ironclaw_memory_mem0/tests/live_local_mem0.rs
  • crates/ironclaw_reborn_cli/src/runtime/mod.rs
  • crates/ironclaw_reborn_composition/src/memory_provider_factory.rs
  • crates/ironclaw_reborn_composition/tests/memory_mem0_swap.rs
  • crates/ironclaw_reborn_config/src/config_file.rs

Comment thread crates/ironclaw_memory_mem0/src/service.rs Outdated
Comment thread crates/ironclaw_reborn_config/src/config_file.rs
@BenKurrek

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_cli/src/runtime/mod.rs (1)

693-702: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Fail loud on unreadable mem0 env settings.

optional_nonempty_env() uses std::env::var(name).ok(), so NotUnicode is treated as “unset”. For MEMORY_MEM0_BASE_URL this can fall back to config, and for MEMORY_MEM0_API_KEY it silently drops auth. Distinguish NotPresent from unreadable values and propagate the latter.

As per path instructions, “Fail loud” flags silent boundary/settings failures.

Suggested fix
-    let mem0_base_url = optional_nonempty_env("MEMORY_MEM0_BASE_URL").or_else(|| {
+    let mem0_base_url = optional_nonempty_env("MEMORY_MEM0_BASE_URL")?.or_else(|| {
         config_file
             .as_ref()
             .and_then(|file| file.memory.as_ref())
             .and_then(|memory| memory.mem0_base_url.clone())
     });
     let memory_provider_connection = ironclaw_reborn_composition::Mem0ConnectionConfig {
         base_url: mem0_base_url,
-        api_key: optional_nonempty_env("MEMORY_MEM0_API_KEY").map(SecretString::from),
-        app_id: optional_nonempty_env("MEMORY_MEM0_APP_ID"),
+        api_key: optional_nonempty_env("MEMORY_MEM0_API_KEY")?.map(SecretString::from),
+        app_id: optional_nonempty_env("MEMORY_MEM0_APP_ID")?,
     };
-fn optional_nonempty_env(name: &str) -> Option<String> {
-    std::env::var(name)
-        .ok()
-        .map(|value| value.trim().to_string())
-        .filter(|value| !value.is_empty())
+fn optional_nonempty_env(name: &str) -> anyhow::Result<Option<String>> {
+    match std::env::var(name) {
+        Ok(value) => Ok(Some(value.trim().to_string()).filter(|value| !value.is_empty())),
+        Err(std::env::VarError::NotPresent) => Ok(None),
+        Err(std::env::VarError::NotUnicode(_)) => {
+            Err(anyhow::anyhow!("{name} must be valid UTF-8"))
+        }
+    }
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_cli/src/runtime/mod.rs` around lines 693 - 702, The
mem0 environment lookup is treating unreadable values as missing, which causes
silent fallback for MEMORY_MEM0_BASE_URL and drops MEMORY_MEM0_API_KEY auth
data. Update optional_nonempty_env (or add a dedicated helper used here in
runtime/mod.rs) to distinguish NotPresent from NotUnicode and return an error
for unreadable env vars instead of converting them to None. Then have the
Mem0ConnectionConfig construction for memory_provider_connection propagate that
error so invalid settings fail loudly rather than falling back to config or
empty credentials.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/ironclaw_reborn_cli/src/runtime/mod.rs`:
- Around line 693-702: The mem0 environment lookup is treating unreadable values
as missing, which causes silent fallback for MEMORY_MEM0_BASE_URL and drops
MEMORY_MEM0_API_KEY auth data. Update optional_nonempty_env (or add a dedicated
helper used here in runtime/mod.rs) to distinguish NotPresent from NotUnicode
and return an error for unreadable env vars instead of converting them to None.
Then have the Mem0ConnectionConfig construction for memory_provider_connection
propagate that error so invalid settings fail loudly rather than falling back to
config or empty credentials.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 55a7d7c2-3186-44d1-8944-01f81c13a281

📥 Commits

Reviewing files that changed from the base of the PR and between a16aa72 and 934b32c.

📒 Files selected for processing (2)
  • crates/ironclaw_reborn_cli/src/runtime/mod.rs
  • crates/ironclaw_reborn_config/src/config_file.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

♻️ Duplicate comments (7)
crates/ironclaw_host_runtime/src/memory_binding.rs (1)

347-353: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Normalize override extension ids before matching.

Line 350 still does a byte-for-byte comparison between the bound extension_id and the admin override. A case-only difference like acme.honcho vs Acme.Honcho makes production resolution reject the override and fail boot even though the extension id is semantically the same. Lowercase both sides once before the any(...) check. As per path instructions, "When comparing user-supplied strings ... extension names, normalize to lowercase with .to_ascii_lowercase() for case-insensitive comparison."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_host_runtime/src/memory_binding.rs` around lines 347 - 353,
The override lookup in memory_binding’s certified-bindings path compares
extension_id byte-for-byte, so case-only differences can prevent a valid admin
override from matching. Normalize both the bound extension_id and each
override.extension_id to lowercase with to_ascii_lowercase() before the any(...)
check in the requires_certified_bindings block, keeping the existing profile_id
and deployment_profile matching logic unchanged.

Source: Path instructions

crates/ironclaw_memory_mem0/src/transport.rs (1)

223-227: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Do not hide malformed mem0 response bodies.

Lines 223-227 still collapse serde_json::from_str(&text) failures into a normal Value::Null response. That masks upstream protocol breaks at an external IO boundary and makes them indistinguishable from an actually empty body. Either propagate the parse error, or keep the null fallback only with an explicit // silent-ok: ... rationale and a debug log of the parse failure. As per path instructions, "Fail loud: flag silent-failure patterns," and as per coding guidelines, "When a fallback is genuinely acceptable, it must be justified inline with a // silent-ok: <reason> comment naming the operation."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_memory_mem0/src/transport.rs` around lines 223 - 227, The
mem0 response parsing in transport.rs is hiding malformed JSON by falling back
to Value::Null in the body construction. Update the response handling around the
serde_json::from_str path to either propagate the parse error from the transport
boundary or, if null is truly acceptable, add an inline // silent-ok: reason
comment naming the parse operation and emit a debug log with the parse failure.
Keep the change localized to the body parsing logic so empty bodies and
malformed bodies are no longer indistinguishable.

Sources: Coding guidelines, Path instructions

crates/ironclaw_reborn_composition/src/lib.rs (1)

231-234: 📐 Maintainability & Code Quality | 🟠 Major | ⚖️ Poor tradeoff

Crate root still re-exports provider-factory seams.

MemoryProviderDeps, Mem0ConnectionConfig, and create_document_store_provider are wiring/test seams, not facade-shaped composition API. As per coding guidelines (ironclaw_reborn_composition: expose facade-shaped handles only; keep lower substrate handles private) and path instructions (no pub use re-exports unless for downstream consumers), keep these pub(crate) and gate any integration-test access behind test-support. build_memory_service_resolver is the intended facade entrypoint.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_composition/src/lib.rs` around lines 231 - 234, The
crate root is re-exporting provider-factory test/wiring seams that should stay
private. Remove the public re-exports of MemoryProviderDeps,
Mem0ConnectionConfig, and create_document_store_provider from the lib.rs facade,
keep them pub(crate) inside memory_provider_factory, and expose any test-only
access through test-support instead; keep build_memory_service_resolver as the
only public composition entrypoint.

Sources: Coding guidelines, Path instructions

crates/ironclaw_reborn_composition/src/local_dev_capability_policy.toml (1)

117-144: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Drop network = "default" from the live native-memory grants.

The live ironclaw.memory.native provider is filesystem-backed only in this PR (the trust entries in factory.rs grant only dispatch + read/write filesystem). Keeping network = "default" on search/write/read/tree hands the always-on memory tools broader authority than the implementation needs. As per coding guidelines (crates/**/*.rs: fail closed for network policy).

Suggested fix
 capability = "ironclaw.memory.native.search"
 effects = ["dispatch_capability", "read_filesystem"]
 mounts = "memory"
-network = "default"
@@
 capability = "ironclaw.memory.native.write"
 effects = ["dispatch_capability", "read_filesystem", "write_filesystem"]
 mounts = "memory"
-network = "default"
@@
 capability = "ironclaw.memory.native.read"
 effects = ["dispatch_capability", "read_filesystem"]
 mounts = "memory"
-network = "default"
@@
 capability = "ironclaw.memory.native.tree"
 effects = ["dispatch_capability", "read_filesystem"]
 mounts = "memory"
-network = "default"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_composition/src/local_dev_capability_policy.toml`
around lines 117 - 144, The live native-memory grants in the local dev policy
are over-permissive by still setting network = "default" on the
ironclaw.memory.native.search, ironclaw.memory.native.write,
ironclaw.memory.native.read, and ironclaw.memory.native.tree entries. Remove the
network setting from these native-memory grant blocks in
local_dev_capability_policy.toml so they match the filesystem-only trust model
used by factory.rs and fail closed for network access.

Source: Coding guidelines

crates/ironclaw_reborn_cli/src/runtime/mod.rs (1)

693-704: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Config-sourced mem0_base_url reaches the connection unchecked.

The env path runs through optional_nonempty_env (trim + non-empty), but the [memory].mem0_base_url fallback is cloned verbatim — no trim, no inline-secret reject. The root cause is the missing config-layer validation flagged in config_file.rs; fixing it there closes this path too. check_base_url in the transport is a last line of defense, not a substitute for the secrets guard.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_cli/src/runtime/mod.rs` around lines 693 - 704, The
config fallback for Mem0 base URL is bypassing the same validation applied to
env values, so fix the config-layer handling in the `config_file` path and
ensure `[memory].mem0_base_url` is trimmed, non-empty validated, and rejected if
it contains inline secrets before it reaches `Mem0ConnectionConfig`. Keep
`optional_nonempty_env` as-is for env vars, but update the config
parsing/validation logic so `memory.mem0_base_url` is normalized and checked
centrally, then let `runtime/mod.rs` continue wiring it into
`with_memory_provider_connection` unchanged.
crates/ironclaw_reborn_composition/src/factory.rs (1)

3284-3287: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Production startup fails open when the bound memory provider can't be built.

build_memory_service_resolver logs and returns a resolver without the third-party provider when mem0 URL/credentials are missing or rejected, so production reports ready while configured memory dispatch fails closed only at invocation time. Per coding guidelines (ironclaw_reborn_composition: production/migration-dry-run must fail closed on missing required handles), convert this to a Result or add a production validation step mapping a missing-selected-provider handle to RebornBuildError::InvalidConfig. Also applies to Lines 3324-3327 (Postgres arm).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_composition/src/factory.rs` around lines 3284 - 3287,
The resolver setup in `build_memory_service_resolver` is allowing startup to
succeed even when the selected third-party memory provider cannot be
constructed, so update the `memory_resolver` path to fail closed by propagating
a `Result` or validating the selected handle and converting missing/invalid
production config into `RebornBuildError::InvalidConfig`. Apply the same
production validation to the Postgres branch as well, and use the existing
`MemoryProviderDeps::for_third_party` and `build_memory_service_resolver` flow
as the entry points for the fix so the unsupported/missing provider is rejected
before readiness is reported.

Source: Coding guidelines

crates/ironclaw_reborn_config/src/config_file.rs (1)

945-980: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

memory.mem0_base_url skips the inline-secret/trim guard.

The memory validation arm checks bindings/overrides but never touches mem0_base_url (Line 132). A credential-bearing or whitespace-only URL pasted here bypasses reject_inline_secret/trim and only fails later at transport construction. As per coding guidelines (crates/**/*.rs: do not expose raw secrets or private URLs across public surfaces).

Suggested fix
         if let Some(memory) = &self.memory {
+            if let Some(mem0_base_url) = &memory.mem0_base_url {
+                check_non_empty_trimmed(Cow::Borrowed("memory.mem0_base_url"), mem0_base_url)?;
+            }
             for (idx, binding) in memory.profile_bindings.iter().enumerate() {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_config/src/config_file.rs` around lines 945 - 980, The
`memory` validation block in `config_file.rs` validates `profile_bindings` and
`admin_overrides` but skips `mem0_base_url`, allowing whitespace-only or
secret-like values to bypass the same trim/inline-secret checks. Update the `if
let Some(memory) = &self.memory` section to validate `memory.mem0_base_url` with
the existing `check_non_empty_trimmed` and `reject_inline_secret` helpers before
validating the bindings/overrides, using the same `memory.mem0_base_url` field
path pattern as the other fields.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/read.md`:
- Around line 1-3: The `read` prompt is incorrectly implying it can return
profile facts, but profile data is accessed through
`MemoryService::profile_read` and not the path-based document `read(path)`
surface. Update the `read.md` wording to describe only persistent document
content for notes/logs/etc., and remove “profile facts” so the prompt stays
aligned with `user_profile_source.rs` and the `MemoryService::profile_read` /
`builtin.profile_set` contract.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.json`:
- Around line 4-8: The output schema for the memory document read response
currently leaves word_count optional, even though NativeMemoryService::read()
and the mem0 read() path always populate it through MemoryServiceReadResponse.
Update the document-read.output.v1.json schema so word_count is included in the
required fields alongside path and content, and keep the field definition
aligned with the existing schema entry to enforce the tool contract.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json`:
- Around line 10-13: The schema descriptions in the document-write output no
longer match the shared document-store contract, so update the
native/profile-level schema to reflect the actual statuses returned by the write
flow. In the document-write output schema and the profile registration in
memory_profiles, revise the field docs for
message/replacements/append/content_length so they describe all valid write
outcomes from the service.rs write path, or split the native-specific and shared
contracts if the meanings differ. Use the document-write.output.v1 schema and
the memory.document_store.v1 profile as the key places to align.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.json`:
- Around line 4-18: Tighten the search request schema in the memory search input
JSON so empty strings and ambiguous alias combinations are rejected at the
boundary. Update the schema around the query-related fields (`query`, `q`,
`text`, `pattern`) to require non-empty values and enforce exactly one alias
key, or else clearly define canonical precedence directly in the schema. Use the
existing search input schema definition to make the validation unambiguous
before downstream normalization.

In `@crates/ironclaw_host_runtime/src/user_profile_source.rs`:
- Around line 50-57: The `from_filesystem` helper in `UserProfileSource` is
bypassing the configured document-store binding by always constructing
`NativeMemoryService` directly. Update this path to use the already-resolved
`Arc<dyn MemoryService>` from `memory_provider`’s fail-closed selection logic,
or rename/restrict `from_filesystem` so it is explicitly native-only and cannot
be used as a generic memory-capability source. Ensure `UserProfileSource::new`
receives the bound `MemoryService` so profile reads and writes stay on the same
backend as `profile_set`.

In `@crates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs`:
- Around line 8612-8617: The native-memory trust wiring in this test harness is
incomplete: trust_policy() is still granting ironclaw.memory.native the full
builtin_effects() ceiling, and provider_trust() only maps builtin, so
CapabilityCatalog::visible_capabilities can omit native providers and hide
least-privilege regressions. Update the trust helpers in
first_party_builtin_tools.rs so native memory has its own trust entry and a
narrower policy derived from its manifest’s declared filesystem effects, and
ensure provider_trust() includes ironclaw.memory.native alongside builtin so
surface tests expose native descriptors correctly.

In `@crates/ironclaw_memory_mem0/src/error.rs`:
- Around line 21-22: The `InvalidUrl` error in `error.rs` is leaking the
configured mem0 URL through its `Display` message. Update the `thiserror`
variant so `InvalidUrl` keeps the `reason` but omits or redacts the raw `url`
value, and make sure any caller constructing or logging this error no longer
exposes the configured URL on public error surfaces.

In `@crates/ironclaw_memory_mem0/src/service.rs`:
- Around line 181-218: The `MemoryService::write` path is silently treating
non-append writes as adds by always calling `Mem0HttpRequest::post(ADD_PATH,
...)` and returning `append: true`. Update `write` to explicitly reject any
request where `request.append` is false, alongside the existing unsupported
patch/empty-content checks, and return an operation/unsupported error instead of
proceeding. Keep the behavior aligned with the other fail-closed branches in
`write`, using the `MemoryServiceWriteRequest` fields and
`MemoryServiceError::operation_from` / `Mem0Error::Unsupported` for the
unsupported case.
- Around line 236-246: The document reconstruction in the mem0 service is using
raw response order, which can scramble multiple fragments for the same target.
Update the logic in response_items/item_metadata_str/item_text handling to sort
the matching items by created_at before building the final content, and use a
stable fallback ordering for items without timestamps so append order is
preserved. Keep the existing filtering by TARGET_KEY and join only after the
ordered list is produced, so the round-trip behavior promised by the module docs
is enforced.

In `@crates/ironclaw_reborn_composition/src/memory_provider_factory.rs`:
- Around line 119-123: The selected mem0 binding can currently be left active
even when no provider was created, because create_document_store_provider and
the resolver wiring in build_memory_service_resolver collapse unsupported ids,
missing config, and transport failures into None. Make this path fallible
instead: return an error when the chosen MemoryProviderBinding cannot be
realized, and propagate that error out of build_memory_service_resolver so
startup fails with context rather than registering a half-initialized resolver.
Use the existing create_document_store_provider and
MemoryProviderBinding/MemoryService symbols to locate the affected flow, and
avoid warn-and-continue behavior that leaves a selected binding without a
provider.

In `@docs/reborn/contracts/memory-profiles.md`:
- Around line 96-100: Update the Non-goals section so it no longer states there
is no Honcho/mem0 provider implementation, since the new mem0 crate and
composition wiring/tests already bind memory.document_store.v1 to mem0. Reword
that bullet to reflect the actual intent, such as that mem0 is not the default
or not certified, and keep the other non-goals unchanged.

---

Duplicate comments:
In `@crates/ironclaw_host_runtime/src/memory_binding.rs`:
- Around line 347-353: The override lookup in memory_binding’s
certified-bindings path compares extension_id byte-for-byte, so case-only
differences can prevent a valid admin override from matching. Normalize both the
bound extension_id and each override.extension_id to lowercase with
to_ascii_lowercase() before the any(...) check in the
requires_certified_bindings block, keeping the existing profile_id and
deployment_profile matching logic unchanged.

In `@crates/ironclaw_memory_mem0/src/transport.rs`:
- Around line 223-227: The mem0 response parsing in transport.rs is hiding
malformed JSON by falling back to Value::Null in the body construction. Update
the response handling around the serde_json::from_str path to either propagate
the parse error from the transport boundary or, if null is truly acceptable, add
an inline // silent-ok: reason comment naming the parse operation and emit a
debug log with the parse failure. Keep the change localized to the body parsing
logic so empty bodies and malformed bodies are no longer indistinguishable.

In `@crates/ironclaw_reborn_cli/src/runtime/mod.rs`:
- Around line 693-704: The config fallback for Mem0 base URL is bypassing the
same validation applied to env values, so fix the config-layer handling in the
`config_file` path and ensure `[memory].mem0_base_url` is trimmed, non-empty
validated, and rejected if it contains inline secrets before it reaches
`Mem0ConnectionConfig`. Keep `optional_nonempty_env` as-is for env vars, but
update the config parsing/validation logic so `memory.mem0_base_url` is
normalized and checked centrally, then let `runtime/mod.rs` continue wiring it
into `with_memory_provider_connection` unchanged.

In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 3284-3287: The resolver setup in `build_memory_service_resolver`
is allowing startup to succeed even when the selected third-party memory
provider cannot be constructed, so update the `memory_resolver` path to fail
closed by propagating a `Result` or validating the selected handle and
converting missing/invalid production config into
`RebornBuildError::InvalidConfig`. Apply the same production validation to the
Postgres branch as well, and use the existing
`MemoryProviderDeps::for_third_party` and `build_memory_service_resolver` flow
as the entry points for the fix so the unsupported/missing provider is rejected
before readiness is reported.

In `@crates/ironclaw_reborn_composition/src/lib.rs`:
- Around line 231-234: The crate root is re-exporting provider-factory
test/wiring seams that should stay private. Remove the public re-exports of
MemoryProviderDeps, Mem0ConnectionConfig, and create_document_store_provider
from the lib.rs facade, keep them pub(crate) inside memory_provider_factory, and
expose any test-only access through test-support instead; keep
build_memory_service_resolver as the only public composition entrypoint.

In `@crates/ironclaw_reborn_composition/src/local_dev_capability_policy.toml`:
- Around line 117-144: The live native-memory grants in the local dev policy are
over-permissive by still setting network = "default" on the
ironclaw.memory.native.search, ironclaw.memory.native.write,
ironclaw.memory.native.read, and ironclaw.memory.native.tree entries. Remove the
network setting from these native-memory grant blocks in
local_dev_capability_policy.toml so they match the filesystem-only trust model
used by factory.rs and fail closed for network access.

In `@crates/ironclaw_reborn_config/src/config_file.rs`:
- Around line 945-980: The `memory` validation block in `config_file.rs`
validates `profile_bindings` and `admin_overrides` but skips `mem0_base_url`,
allowing whitespace-only or secret-like values to bypass the same
trim/inline-secret checks. Update the `if let Some(memory) = &self.memory`
section to validate `memory.mem0_base_url` with the existing
`check_non_empty_trimmed` and `reject_inline_secret` helpers before validating
the bindings/overrides, using the same `memory.mem0_base_url` field path pattern
as the other fields.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3042703e-1241-405f-96ad-8d60d5af5069

📥 Commits

Reviewing files that changed from the base of the PR and between c02f73d and 934b32c.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock
📒 Files selected for processing (66)
  • Cargo.toml
  • crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs
  • crates/ironclaw_host_api/src/host_port.rs
  • crates/ironclaw_host_runtime/assets/memory_native/manifest.toml
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/read.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/search.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/tree.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/write.md
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.json
  • crates/ironclaw_host_runtime/src/extension_contracts.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/memory.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/mod.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs
  • crates/ironclaw_host_runtime/src/lib.rs
  • crates/ironclaw_host_runtime/src/memory_binding.rs
  • crates/ironclaw_host_runtime/src/memory_context.rs
  • crates/ironclaw_host_runtime/src/memory_native_extension.rs
  • crates/ironclaw_host_runtime/src/memory_profiles.rs
  • crates/ironclaw_host_runtime/src/memory_provider.rs
  • crates/ironclaw_host_runtime/src/surface.rs
  • crates/ironclaw_host_runtime/src/user_profile_source.rs
  • crates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs
  • crates/ironclaw_host_runtime/tests/memory_capability_profiles.rs
  • crates/ironclaw_host_runtime/tests/memory_native_manifest.rs
  • crates/ironclaw_host_runtime/tests/memory_native_schema_validation.rs
  • crates/ironclaw_host_runtime/tests/memory_prompt_context.rs
  • crates/ironclaw_host_runtime/tests/user_profile_roundtrip.rs
  • crates/ironclaw_memory/src/lib.rs
  • crates/ironclaw_memory/src/service.rs
  • crates/ironclaw_memory_mem0/Cargo.toml
  • crates/ironclaw_memory_mem0/src/config.rs
  • crates/ironclaw_memory_mem0/src/error.rs
  • crates/ironclaw_memory_mem0/src/lib.rs
  • crates/ironclaw_memory_mem0/src/service.rs
  • crates/ironclaw_memory_mem0/src/transport.rs
  • crates/ironclaw_memory_mem0/src/url_check.rs
  • crates/ironclaw_memory_mem0/tests/live_local_mem0.rs
  • crates/ironclaw_memory_native/src/service.rs
  • crates/ironclaw_memory_native/tests/memory_service_facade.rs
  • crates/ironclaw_reborn_cli/src/runtime/mod.rs
  • crates/ironclaw_reborn_composition/Cargo.toml
  • crates/ironclaw_reborn_composition/src/factory.rs
  • crates/ironclaw_reborn_composition/src/input.rs
  • crates/ironclaw_reborn_composition/src/lib.rs
  • crates/ironclaw_reborn_composition/src/local_dev_capability_policy.toml
  • crates/ironclaw_reborn_composition/src/memory_binding.rs
  • crates/ironclaw_reborn_composition/src/memory_provider_factory.rs
  • crates/ironclaw_reborn_composition/src/projection/display_preview.rs
  • crates/ironclaw_reborn_composition/src/projection/tests/display_preview.rs
  • crates/ironclaw_reborn_composition/src/runtime.rs
  • crates/ironclaw_reborn_composition/src/runtime/local_dev.rs
  • crates/ironclaw_reborn_composition/tests/memory_mem0_swap.rs
  • crates/ironclaw_reborn_config/src/config_file.rs
  • crates/ironclaw_reborn_config/src/lib.rs
  • docs/adr/0001-capability-manifest-v2-hard-cutover.md
  • docs/adr/0002-native-memory-uses-host-storage-ports.md
  • docs/reborn/contracts/memory-profiles.md
  • tests/reborn_trace_first_party_tool_coverage.rs
  • tests/support/reborn/harness.rs

Comment thread crates/ironclaw_host_runtime/src/user_profile_source.rs
Comment thread crates/ironclaw_memory_mem0/src/error.rs Outdated
Comment thread crates/ironclaw_memory_mem0/src/service.rs
Comment thread crates/ironclaw_memory_mem0/src/service.rs
Comment thread crates/ironclaw_reborn_composition/src/memory_provider_factory.rs
Comment thread docs/reborn/contracts/memory-profiles.md
BenKurrek and others added 3 commits June 25, 2026 21:54
…ce-wide allowlist

PR #5163 made only the memory rules allowlist-based; the other ~30
`BoundaryRule` entries stayed blocklists that under-enforce — they forbid
today's offenders but would silently admit a future internal dep (e.g.
`ironclaw_turns`, `ironclaw_product_workflow`, `ironclaw_reborn`).

Convert the whole harness to an allowlist:

- `BoundaryRule` now carries `allowed: Vec<&'static str>` instead of
  `forbidden`. The runner computes
  `forbidden = workspace_ironclaw_crates() - allowed - crate_name`, so any
  unlisted internal dependency now fails the boundary test.
- Each crate's `allowed` set is its actual normal `ironclaw_*` dependencies,
  matching the dependency guardrail documented in its CLAUDE.md/AGENTS.md.
- The three previously-inline allowlist rules (`ironclaw_host_api`,
  `ironclaw_memory`, `ironclaw_memory_native`) are folded into
  `boundary_rules()` so the test body is a single uniform loop.

Behavior-preserving on the current (correct) dependency graph; the win is
forward enforcement. Addresses serrrfirat's deferred thread on #5163
(discussion_r3468163078).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ontext

Before this change the native provider sanitized, wrapped, and hashed each
memory snippet, and the host only *asserted* the `Untrusted memory content:`
prefix in `admit_memory_context_snippet`. A future untrusted provider could
pre-attach that prefix (or pre-shape the snippet) and slip text past the host's
prompt-safety wrapper.

Move all model-visible shaping into the host so the provider can never bypass
prompt safety:

- `MemoryServiceContextSnippet` now carries RAW snippet text plus the resolved
  scope/path components (`tenant_id`, `user_id`, `agent_id`, `project_id`,
  `relative_path`) — no `snippet_ref`/`safe_summary`/`model_content`.
- The native `retrieve_context` ranks and scope-filters candidates, then returns
  them raw; it no longer sanitizes, truncates, hashes, or budgets. Removed the
  native `sanitize_snippet_text`, `truncate_to_char_boundary`,
  `validate_loop_safe_summary`, `memory_snippet_display_ref`, `feed_hash`,
  `collect_context_snippets`, the FNV/budget consts, and the prompt-envelope dep.
- The host `memory_context.rs` builds the `memory-snippet:*` reference via the
  canonical `ironclaw_turns::run_profile::memory_snippet_display_ref`, sanitizes
  + wraps the raw text (`sanitize_snippet_text` relocated here), validates through
  the loop's own `LoopSafeSummary` gate (collapsing the native denylist copy into
  one source of truth), and enforces the per-snippet (512B) + aggregate (4 KiB)
  budgets in the admission loop with the same break semantics the native
  `collect_context_snippets` used.

Behavior-preserving for the native provider: model-visible output is byte-for-byte
identical (same wrapping, same FNV trailing-separator
`memory-snippet:cb96ed00b13e6ae4` golden ref, same caps/ordering). New coverage
proves a provider that returns text merely starting with the untrusted prefix is
STILL re-sanitized + re-wrapped by the host
(`adapter_re_sanitizes_provider_supplied_untrusted_prefix`,
`sanitize_re_wraps_text_already_carrying_untrusted_prefix`), plus the legacy ref
golden lock and the host-owned aggregate-budget test.

Addresses serrrfirat's deferred security thread on #5163 (discussion_r3468163070;
ref-stability discussion_r3466587649).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…facade

Profile READS built the native repository/backend directly in
`user_profile_source.rs` and duplicated the scope/path decision
(`profile_scope_and_path` + `PROFILE_DOCUMENT_PATH`) that
`NativeMemoryService` already owns for WRITES (`profile_set`). That coupled
profile reads to the concrete native provider and left provider selection unable
to swap reads with the rest of the memory facade.

Add a provider-neutral profile read to the contract and route the host read
through it:

- New `MemoryService::profile_read(invocation) -> MemoryServiceProfileReadResponse`
  (raw document bytes) on the `ironclaw_memory` trait, with a native
  implementation that reuses the SAME `profile_scope_and_path` as `profile_set`
  — so the scope/path decision lives in exactly one place per provider.
- `MemoryBackedUserProfileSource` now holds `Arc<dyn MemoryService>` and reads
  via `profile_read`; the host keeps only the parse + 64 KiB size-cap +
  validation. Deleted the duplicate host `profile_scope_and_path` /
  `PROFILE_DOCUMENT_PATH` and their re-export.
- Production wiring uses the new `MemoryBackedUserProfileSource::from_filesystem`
  factory (host owns the native-provider choice, matching the memory capability);
  the composition layer keeps passing the workspace filesystem.

Behavior-preserving: the unit tests assert identical parse/validation outcomes
(now through a stub `MemoryService`), and the end-to-end
`user_profile_roundtrip` test still proves the agent-scoped write → user-scoped
read round trip, now through `MemoryService::profile_read`.

Addresses serrrfirat's deferred thread on #5163 (discussion_r3466587663).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
BenKurrek and others added 7 commits June 25, 2026 21:54
… ids

The projection display-preview summarizer keys on the `memory_<op>` short names
via `capability_matches`, which matched `builtin.memory_<op>` by suffix. The
native tools are now `ironclaw.memory.native.<op>` (suffix `.<op>`, not
`.memory_<op>`), so memory input summaries — including write-content secret
redaction — would have silently stopped rendering in production. Teach
`capability_matches` the new id shape and update the projection test fixtures to
the native ids so they exercise it. Also rename the now-misnamed
`builtin_memory_search_dispatches_*` host_runtime test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…untime

The native-memory consolidation (ffbd298, dc2fbb5) moved the memory_*
capabilities out of the builtin package into the always-on
`ironclaw.memory.native` package and reshaped the bundled input schemas to the
four live document-store tools, but the host_runtime test suite + two schemas
were left asserting the old shape — leaving `Test ironclaw_host_runtime` red on
18 memory tests.

- first_party_builtin_tools.rs: resolve the memory capabilities from the native
  package (register it in the test registry + add its first-party trust entry)
  and drop the memory ids from `all_builtin_capability_ids`.
- memory_native_schema_validation.rs: validate the four live tool schemas
  (read/write/search/tree); the removed context-retrieve / interaction-record
  schemas belong to the deferred host-managed flow and are no longer bundled.
- search.input.v1.json: accept the `q`/`text`/`pattern` aliases that
  `MemoryServiceSearchRequest::from_tool_input` already honors (anyOf), so a
  model call using an alias is not rejected at the pre-dispatch schema boundary.
- document-read.input.v1.json: reject empty and absolute paths at the schema
  (minLength + `^[^/]` pattern), matching the scoped-path contract.
- ironclaw_memory service.rs: restore the pre-lift null-target handling — an
  explicit JSON `null` write target is treated as omitted (daily_log) rather
  than rejected, the #4547 behavior the lift had regressed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ackage

Moving the memory_* capabilities into the `ironclaw.memory.native` package left
two root-crate tests red, because the e2e harness and the e2e coverage allowlist
still assumed memory lived in `builtin`:

- reborn_trace_first_party_tool_coverage.rs: build the covered-capability set
  from the union of the builtin and native-memory packages, so the always-on
  first-party surface (which now spans two packages) is fully checked.
- tests/support/reborn/harness.rs: register `native_memory_first_party_package`
  in the core-builtins runtime (via a shared `core_builtins_extension_registry`
  so the two core-builtins runtimes cannot drift), trust the native provider at
  the host-policy and per-run authority levels, and scope memory grants to
  filesystem effects so they fit the native provider's tight authority ceiling.

Fixes `reborn_builtin_first_party_capability_e2e_coverage_is_complete` and
`reborn_trace_memory_first_party_tools_parity` on `Reborn root tests` and
`Tests (all-features)`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make the memory document-store provider swappable to mem0 entirely through
config, with no hardcoded native assumption in the kernel — demonstrating the
#3537 memory architecture is genuinely pluggable. Follows the established
`ironclaw_embeddings::create_provider` config-driven-factory idiom.

- ironclaw_host_runtime/memory_provider.rs: remove the hardwired
  native-or-none logic; `MemoryServiceResolver` is now a provider-agnostic
  registry (`BTreeMap<extension_id, Arc<dyn MemoryService>>` + a
  `with_third_party_document_store_provider` builder). `resolve_document_store`
  matches the binding (Native -> build native / ThirdParty(id) -> registered
  instance, None if unregistered / Disabled -> None). It names no concrete
  third-party provider, so host_runtime keeps zero provider deps.
- crates/ironclaw_memory_mem0: new provider crate implementing MemoryService
  over the mem0 REST API. Real reqwest transport behind a `Mem0Transport`
  trait (SSRF-checked base URL, `Authorization: Token` header) with a
  panic-free mock for tests. Depends only on ironclaw_memory + ironclaw_host_api.
- ironclaw_reborn_composition: `create_document_store_provider(binding, deps)`
  factory (embeddings idiom: match Native/ThirdParty/Disabled, build mem0 over
  its real transport with check_base_url, fail-closed None on missing creds)
  plus `build_memory_service_resolver` that registers third-party providers;
  wired into all three resolver-construction sites in factory.rs.
- Config: `[memory] mem0_base_url` + env MEMORY_MEM0_{API_KEY,BASE_URL,APP_ID}
  (API key as SecretString), mirroring EmbeddingsConfig.
- Tests: end-to-end swap proof (config -> policy -> factory -> registry ->
  resolve_document_store returns mem0, not native; write+search route through
  the mem0 mock transport), mem0 unit tests, and a caller-level tool-dispatch
  test proving the unchanged ironclaw.memory.native.* tools transparently route
  to mem0 under a binding. Architecture boundary allowlist updated for the new
  crate (composition may depend on it; host_runtime may not).

Scope: document-store swap only. The host-managed retrieve/record lifecycle and
SQL storage-port backing remain the deferred #5264 follow-ups.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a full-pipeline (`load_memory_snippets`) negative test proving the host
drops memory-context snippets whose resolved tenant/user scope does not match
the request scope, even when the provider returns them — keeping only the
in-scope snippet. The scope guard was unit-tested at the `admit_*` level; this
exercises it end-to-end against a malicious or buggy provider, which is now a
live possibility with config-bound third-party providers like mem0 (#5264).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-target the mem0 document-store provider from mem0's hosted cloud API to a
self-hosted mem0 open-source instance on localhost — no dependency on
api.mem0.ai and no cloud API key. Proven end-to-end against a real local stack
(mem0ai + Qdrant + an Ollama embedder): store -> search-recall -> verbatim read
round-trips, with the data physically in the local vector store.

- transport.rs: the API key is now optional (`Option<&str>` — the auth header is
  sent only when a key is present); add `.timeout(30s)` + `.redirect(none)`
  (review hardening, finding #2).
- service.rs: local OSS paths (`/memories`, `/search`, `GET /memories?user_id=`)
  instead of the hosted `/v1/memories/...`; `add` sends `infer:false` so mem0
  stores content verbatim (document-store semantics need only the embedder, not
  the extraction LLM). `profile_set` is now field-preserving (read-merge-write,
  latest selected by `created_at`) instead of last-writer-wins (review finding
  #1 — no more silent profile-field loss); merge + infer-false unit tests added.
- lib.rs: extension id `mem0.cloud.memory` -> `mem0.local.memory`; docs rewritten
  to the local OSS surface.
- composition factory: build the provider with an optional key (no longer fails
  closed on a missing key); reborn_cli defaults `MEMORY_MEM0_BASE_URL` to
  `http://localhost:8888`; config doc updated.
- swap-test fixtures updated to the local API; new `tests/live_local_mem0.rs`
  (`#[ignore]`'d) drives the real transport against a running local mem0.

The document-store swap is proven at the provider level. The full LLM-agent loop
using memory remains blocked on this branch by the pre-existing #5206
worker-pool stall (fixed on main) and is a tracked follow-up (#5264).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the `http://localhost:8888` default for the mem0 connection base URL so
mem0 is fully opt-in: it activates only when an operator both binds the
document-store profile to it AND supplies a base URL (the `[memory]` config or
`MEMORY_MEM0_BASE_URL`). A bound-but-unconfigured mem0 fails closed in the
factory, and the binding policy already defaults to native — so the shipped
default memory layer is unchanged (native filesystem); mem0 never engages
unless explicitly configured.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@BenKurrek BenKurrek force-pushed the reborn/memory-lift-followups branch from 934b32c to 2d21441 Compare June 26, 2026 01:55
BenKurrek and others added 2 commits June 25, 2026 22:01
…loud, tests)

From a 5-agent end-to-end review of the PR. All low-risk:
- mem0 provider: reject embedded-credential base URLs (redacted in the error) and
  run `memory.mem0_base_url` through the config inline-secret guard; correct the
  stale "defaults to localhost:8888" doc-comments (there is no default — a
  bound-but-unset mem0 fails closed); add the missing `created_at` newest-profile
  test; fail loud (`CorruptProfile`) instead of silently dropping fields when an
  existing profile blob is unparseable; add the `// silent-ok:` annotation and
  drop cloud (`api.mem0.ai`, `/v1/`) remnants from test/doc strings.
- reborn_cli: `optional_nonempty_env` fails loud on a non-UTF-8 value
  (NotPresent -> None, NotUnicode -> Err) for the three MEMORY_MEM0_* reads.
- reborn_config: deployment-profile validation uses `RebornProfile::from_str`
  instead of matching string literals (types.md).
- architecture: add a boundary-test guard asserting host_runtime stays
  memory-provider-neutral (only composition may name `ironclaw_memory_mem0`).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A third-party document_store binding (e.g. mem0) does not get write-time prompt-write-safety enforcement or per-write audit, since that engine lives inside the native provider. Spell out the security limitation in resolve_document_store + why it is acceptable for the off-by-default surface (third parties cannot reach the trusted prompt surface; all retrieved content is host-wrapped untrusted), and that hoisting it host-side is deferred to #5264.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

♻️ Duplicate comments (5)
crates/ironclaw_memory_mem0/src/service.rs (4)

198-218: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Non-append writes still silently become adds.

request.append is ignored: a replace-style write (append=false, no patch args) emits POST /memories and returns append: true, leaving stale fragments that read() later concatenates. Fail closed as unsupported, alongside the patch/clear guards. As per path instructions, flag silent-failure patterns.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_memory_mem0/src/service.rs` around lines 198 - 218, The write
path in `MemoryService::write` ignores `request.append`, so replace-style writes
still go through the additive `POST /memories` flow and return `append: true`.
Update the guards near the existing patch/clear handling to fail closed for
non-append writes by returning an unsupported operation error when `append` is
false and no patch args are present, instead of silently treating the request as
an add.

Source: Path instructions


226-242: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

read/tree/profile_read still consume only the first /memories page.

No page/next follow, so entries past page 1 are invisible — a valid document/profile reads back as a false miss or stale result. Paginate until exhausted before reconstructing.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_memory_mem0/src/service.rs` around lines 226 - 242, The read
path in the memory service only processes the first LIST_PATH response, so
`read`, `tree`, and `profile_read` can miss entries beyond page 1. Update the
logic in `MemoryService::read` to keep following pagination metadata from the
`/memories` response (for example via `page`/`next`) until all pages are
exhausted, then rebuild the document/profile from the full accumulated result
set using the existing `response_items`, `item_metadata_str`, and `item_text`
helpers.

140-142: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Still resetting malformed profiles to empty — fail loud.

latest_profile_text(...).and_then(|t| serde_json::from_str(t).ok()).unwrap_or_default() collapses a parse error into an empty object, so the next profile_set overwrites existing fields. Distinguish "no prior profile" from "parse failed" and propagate the error (or add a // silent-ok: rationale). As per path instructions, flag silent-failure patterns.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_memory_mem0/src/service.rs` around lines 140 - 142, The
profile loading path in service.rs is silently treating malformed profile JSON
as an empty map by chaining latest_profile_text, serde_json::from_str, and
unwrap_or_default. Update the logic in the profile retrieval flow to distinguish
“no existing profile text” from “parse failed”: return/propagate an error when
deserialization fails instead of defaulting to empty, or add an explicit
silent-ok rationale if that behavior is truly intended. Use the existing
latest_profile_text and profile_set flow as the place to fix this so malformed
profiles are not overwritten.

Source: Path instructions


238-246: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

read still joins fragments in raw list order.

mem0 list order isn't chronological (the module docs say so), so two writes to the same target can reassemble out of order, breaking the round-trip guarantee. Sort matching items by created_at (stable index fallback) before join.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_memory_mem0/src/service.rs` around lines 238 - 246, The read
path currently rebuilds content by joining matching fragments in the raw mem0
list order, which can return writes out of sequence. Update the logic in the
service read flow that builds `parts` from `response_items(&response.body)` to
order matching items by `created_at` first, with a stable original-index
fallback for ties or missing timestamps, before calling `join`. Keep the filter
on `TARGET_KEY` and the text extraction via `item_text`, but ensure the final
`content` is assembled from the sorted fragments so round-tripping multiple
writes to the same target remains deterministic.
crates/ironclaw_reborn_config/src/config_file.rs (1)

945-980: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

memory.mem0_base_url is still unvalidated.

The block validates bindings/overrides but skips memory.mem0_base_url, so a whitespace-only or credential-bearing URL bypasses the inline-secret/trim guard every other operator string gets and fails later at provider startup. Run it through check_non_empty_trimmed. As per coding guidelines, crates/**/*.rs must not expose raw secrets or private URLs across public surfaces.

Suggested fix
         if let Some(memory) = &self.memory {
+            if let Some(mem0_base_url) = &memory.mem0_base_url {
+                check_non_empty_trimmed(Cow::Borrowed("memory.mem0_base_url"), mem0_base_url)?;
+            }
             for (idx, binding) in memory.profile_bindings.iter().enumerate() {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_config/src/config_file.rs` around lines 945 - 980, The
memory validation block in config_file.rs checks profile_bindings and
admin_overrides but still skips memory.mem0_base_url, allowing invalid or
secret-bearing values to pass through. Update the existing validation in the
memory section to run mem0_base_url through check_non_empty_trimmed alongside
the other string fields, keeping the same trim/empty guard pattern used for
profile_id, extension_id, and deployment_profile.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_host_api/src/host_port.rs`:
- Around line 21-25: The docstring on the host port contract overstates a live
guarantee by claiming it backs native-memory persistence; soften this language
to describe intended use rather than an enforced cross-layer contract. Update
the comment on the host port type in host_port.rs so it aligns with the current
provider behavior documented by native_memory_declares_no_host_ports() in
memory_native_extension.rs, and avoid wording that implies native-memory storage
is currently required or guaranteed. Keep the description focused on the
validation/contract role of the port and the fact that concrete adapters live in
host/runtime service crates.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.input.v1.json`:
- Around line 4-8: The `path` field in `tree.input.v1.json` is still too
permissive and allows absolute, traversal, and backslash-based paths instead of
failing closed at the schema boundary. Tighten the JSON schema for `path` so it
only accepts scoped relative segments (with `""` as the sole root sentinel), and
update the `path` property definition to reject `/abs`, `../escape`, and
backslash variants before they reach the runtime.

In `@crates/ironclaw_host_runtime/src/memory_binding.rs`:
- Around line 271-288: The binding selection logic in the profile loop should
normalize the configured `extension_id` before comparing it against
`NATIVE_MEMORY_EXTENSION_ID` and `MEMORY_DISABLED_BINDING_SENTINEL`, since raw
case-sensitive checks can misclassify user input. Update the matching in
`memory_binding.rs` around `configured.get(profile)` so comparisons use a
lowercased form of the config value, while keeping `resolve_third_party`,
`native_binding`, and the disabled-production path behavior unchanged.

In `@crates/ironclaw_host_runtime/src/memory_provider.rs`:
- Around line 33-59: The resolver currently downcasts validated extension
identifiers to raw strings in DocumentStoreResolution::ThirdParty and
MemoryServiceResolver::third_party_document_store, which can let registration
and lookup drift. Update the resolver to carry ExtensionId through the enum and
registry, and adjust the related resolution/registration paths in
MemoryServiceResolver so lookup keys and binding results stay type-safe and
cannot vary by invalid or case-variant strings.

In `@crates/ironclaw_host_runtime/tests/memory_native_schema_validation.rs`:
- Around line 38-56: The current test only validates that schema files compile,
but it does not exercise the live native-schema lookup path used at runtime. Add
a caller-level test that drives each native capability through
CapabilityCatalog::surface_descriptor so resolve_native_memory_input_schema_ref
is actually used, and make the test fail if any manifest.toml schema ref does
not match the hard-coded arms. Keep the existing compilation check, but extend
the suite with a real call-site validation around native_memory_manifest and
CapabilityCatalog to catch ref drift where the runtime would.

In `@crates/ironclaw_memory_mem0/src/transport.rs`:
- Around line 157-188: Reject URLs containing userinfo before constructing
Mem0HttpTransport: update check_base_url() to fail when the parsed base URL
includes a non-empty username or password, so Mem0HttpTransport::new never
stores sensitive inline credentials in base_url or exposes them through Debug.
Keep the validation centralized in the base-URL check and return a
Mem0Error::Client with a clear reason when userinfo is present.

In `@crates/ironclaw_memory_mem0/src/url_check.rs`:
- Around line 28-57: The Mem0 URL validation is currently too permissive and
still allows hosted cloud endpoints like api.mem0.ai, which should be rejected
up front. Update check_base_url and the related accepts_normal_endpoints logic
to fail closed for known hosted/non-self-hosted Mem0 endpoints, and ensure
Mem0Error::InvalidUrl is returned immediately with a clear reason so invalid
configs are blocked before any request is made.

In `@crates/ironclaw_reborn_composition/src/memory_provider_factory.rs`:
- Around line 47-50: The mem0 base URL contract is inconsistent between the
`MemoryProviderFactory` docs and the actual `base_url` handling. Update the
`base_url` field documentation and any related comments around
`MemoryProviderFactory`/CLI wiring so it clearly states there is no implicit
localhost default and mem0 is not auto-enabled when `base_url` is absent. Keep
the behavior in the factory logic unchanged and align the docs with the
fail-closed path used when `base_url` is missing.

---

Duplicate comments:
In `@crates/ironclaw_memory_mem0/src/service.rs`:
- Around line 198-218: The write path in `MemoryService::write` ignores
`request.append`, so replace-style writes still go through the additive `POST
/memories` flow and return `append: true`. Update the guards near the existing
patch/clear handling to fail closed for non-append writes by returning an
unsupported operation error when `append` is false and no patch args are
present, instead of silently treating the request as an add.
- Around line 226-242: The read path in the memory service only processes the
first LIST_PATH response, so `read`, `tree`, and `profile_read` can miss entries
beyond page 1. Update the logic in `MemoryService::read` to keep following
pagination metadata from the `/memories` response (for example via
`page`/`next`) until all pages are exhausted, then rebuild the document/profile
from the full accumulated result set using the existing `response_items`,
`item_metadata_str`, and `item_text` helpers.
- Around line 140-142: The profile loading path in service.rs is silently
treating malformed profile JSON as an empty map by chaining latest_profile_text,
serde_json::from_str, and unwrap_or_default. Update the logic in the profile
retrieval flow to distinguish “no existing profile text” from “parse failed”:
return/propagate an error when deserialization fails instead of defaulting to
empty, or add an explicit silent-ok rationale if that behavior is truly
intended. Use the existing latest_profile_text and profile_set flow as the place
to fix this so malformed profiles are not overwritten.
- Around line 238-246: The read path currently rebuilds content by joining
matching fragments in the raw mem0 list order, which can return writes out of
sequence. Update the logic in the service read flow that builds `parts` from
`response_items(&response.body)` to order matching items by `created_at` first,
with a stable original-index fallback for ties or missing timestamps, before
calling `join`. Keep the filter on `TARGET_KEY` and the text extraction via
`item_text`, but ensure the final `content` is assembled from the sorted
fragments so round-tripping multiple writes to the same target remains
deterministic.

In `@crates/ironclaw_reborn_config/src/config_file.rs`:
- Around line 945-980: The memory validation block in config_file.rs checks
profile_bindings and admin_overrides but still skips memory.mem0_base_url,
allowing invalid or secret-bearing values to pass through. Update the existing
validation in the memory section to run mem0_base_url through
check_non_empty_trimmed alongside the other string fields, keeping the same
trim/empty guard pattern used for profile_id, extension_id, and
deployment_profile.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4dce505d-897a-45d6-9af8-f60df2207311

📥 Commits

Reviewing files that changed from the base of the PR and between 934b32c and 2d21441.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock
📒 Files selected for processing (66)
  • Cargo.toml
  • crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs
  • crates/ironclaw_host_api/src/host_port.rs
  • crates/ironclaw_host_runtime/assets/memory_native/manifest.toml
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/read.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/search.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/tree.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/write.md
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.json
  • crates/ironclaw_host_runtime/src/extension_contracts.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/memory.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/mod.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs
  • crates/ironclaw_host_runtime/src/lib.rs
  • crates/ironclaw_host_runtime/src/memory_binding.rs
  • crates/ironclaw_host_runtime/src/memory_context.rs
  • crates/ironclaw_host_runtime/src/memory_native_extension.rs
  • crates/ironclaw_host_runtime/src/memory_profiles.rs
  • crates/ironclaw_host_runtime/src/memory_provider.rs
  • crates/ironclaw_host_runtime/src/surface.rs
  • crates/ironclaw_host_runtime/src/user_profile_source.rs
  • crates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs
  • crates/ironclaw_host_runtime/tests/memory_capability_profiles.rs
  • crates/ironclaw_host_runtime/tests/memory_native_manifest.rs
  • crates/ironclaw_host_runtime/tests/memory_native_schema_validation.rs
  • crates/ironclaw_host_runtime/tests/memory_prompt_context.rs
  • crates/ironclaw_host_runtime/tests/user_profile_roundtrip.rs
  • crates/ironclaw_memory/src/lib.rs
  • crates/ironclaw_memory/src/service.rs
  • crates/ironclaw_memory_mem0/Cargo.toml
  • crates/ironclaw_memory_mem0/src/config.rs
  • crates/ironclaw_memory_mem0/src/error.rs
  • crates/ironclaw_memory_mem0/src/lib.rs
  • crates/ironclaw_memory_mem0/src/service.rs
  • crates/ironclaw_memory_mem0/src/transport.rs
  • crates/ironclaw_memory_mem0/src/url_check.rs
  • crates/ironclaw_memory_mem0/tests/live_local_mem0.rs
  • crates/ironclaw_memory_native/src/service.rs
  • crates/ironclaw_memory_native/tests/memory_service_facade.rs
  • crates/ironclaw_reborn_cli/src/runtime/mod.rs
  • crates/ironclaw_reborn_composition/Cargo.toml
  • crates/ironclaw_reborn_composition/src/factory.rs
  • crates/ironclaw_reborn_composition/src/input.rs
  • crates/ironclaw_reborn_composition/src/lib.rs
  • crates/ironclaw_reborn_composition/src/local_dev_capability_policy.toml
  • crates/ironclaw_reborn_composition/src/memory_binding.rs
  • crates/ironclaw_reborn_composition/src/memory_provider_factory.rs
  • crates/ironclaw_reborn_composition/src/projection/display_preview.rs
  • crates/ironclaw_reborn_composition/src/projection/tests/display_preview.rs
  • crates/ironclaw_reborn_composition/src/runtime.rs
  • crates/ironclaw_reborn_composition/src/runtime/local_dev.rs
  • crates/ironclaw_reborn_composition/tests/memory_mem0_swap.rs
  • crates/ironclaw_reborn_config/src/config_file.rs
  • crates/ironclaw_reborn_config/src/lib.rs
  • docs/adr/0001-capability-manifest-v2-hard-cutover.md
  • docs/adr/0002-native-memory-uses-host-storage-ports.md
  • docs/reborn/contracts/memory-profiles.md
  • tests/reborn_trace_first_party_tool_coverage.rs
  • tests/support/reborn/harness.rs
💤 Files with no reviewable changes (5)
  • tests/reborn_trace_first_party_tool_coverage.rs
  • docs/adr/0002-native-memory-uses-host-storage-ports.md
  • docs/adr/0001-capability-manifest-v2-hard-cutover.md
  • docs/reborn/contracts/memory-profiles.md
  • tests/support/reborn/harness.rs

Comment thread crates/ironclaw_host_api/src/host_port.rs Outdated
Comment thread crates/ironclaw_host_runtime/src/memory_binding.rs
Comment thread crates/ironclaw_host_runtime/src/memory_provider.rs
Comment thread crates/ironclaw_memory_mem0/src/transport.rs
Comment thread crates/ironclaw_memory_mem0/src/url_check.rs
Comment thread crates/ironclaw_reborn_composition/src/memory_provider_factory.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/ironclaw_memory_mem0/src/url_check.rs (1)

34-52: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Redact before any parsed-URL rejection.

The userinfo guard runs after the scheme check, so ftp://operator:s3cr3t-token@mem0.example.com takes Lines 35-39 and Mem0Error::InvalidUrl formats the secret-bearing URL. Build a redacted display string immediately after parse and use it for all parsed invalid-url errors; add that regression next to rejects_embedded_credentials. As per coding guidelines, crates/**/*.rs must not expose raw secrets or private URLs across public surfaces.

Suggested fix
     let parsed = reqwest::Url::parse(url).map_err(|error| Mem0Error::InvalidUrl {
         url: url.to_string(),
         reason: error.to_string(),
     })?;
+    let redacted_url = redact_userinfo(&parsed);

     let scheme = parsed.scheme();
     if scheme != "http" && scheme != "https" {
         return Err(Mem0Error::InvalidUrl {
-            url: url.to_string(),
+            url: redacted_url.clone(),
             reason: format!("only http/https are allowed (got '{scheme}')"),
         });
     }

@@
     if !parsed.username().is_empty() || parsed.password().is_some() {
         return Err(Mem0Error::InvalidUrl {
-            url: redact_userinfo(&parsed),
+            url: redacted_url,
             reason: "must not embed credentials in the base URL (userinfo is not allowed)"
                 .to_string(),
         });
     }

Also applies to: 118-133

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_memory_mem0/src/url_check.rs` around lines 34 - 52, The
parsed URL rejection path in url validation still emits the original
secret-bearing URL before the credentials guard runs. Update url handling in
url_check.rs so a redacted display string is created immediately after parsing
and reused for every Mem0Error::InvalidUrl case, including the scheme check and
the embedded-credentials check in the url validation logic. Keep the redaction
centralized around redact_userinfo and add a regression test alongside
rejects_embedded_credentials that covers a non-http/https URL with userinfo to
ensure no raw secrets appear in errors.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_reborn_cli/src/runtime/mod.rs`:
- Around line 779-783: The Google OAuth env lookup in
resolve_google_oauth_config is swallowing malformed values by converting
optional_nonempty_env(name).ok().flatten() to None, which can silently change
auth setup. Update the resolver path to propagate the env read error instead of
discarding it, preserving the existing startup-fail behavior for non-UTF-8 or
otherwise invalid OAuth overrides. If product truly wants to ignore malformed
OAuth env vars, add the required silent-ok exception annotation on this path;
otherwise keep the error flow loud by default.

In `@crates/ironclaw_reborn_config/src/config_file.rs`:
- Around line 985-986: `mem0_base_url` is only being checked for secret
patterns, so whitespace-only or padded URLs can still slip through and fail
later; update the `memory.mem0_base_url` validation in `config_file.rs` to use
the same `check_non_empty_trimmed` guard as the other memory fields, then keep
the existing `check(...)` secret scan after trimming. Add or extend the
whitespace regression tests around the `parse_text`/config validation path to
cover both blank and surrounding-whitespace `mem0_base_url` values.

---

Outside diff comments:
In `@crates/ironclaw_memory_mem0/src/url_check.rs`:
- Around line 34-52: The parsed URL rejection path in url validation still emits
the original secret-bearing URL before the credentials guard runs. Update url
handling in url_check.rs so a redacted display string is created immediately
after parsing and reused for every Mem0Error::InvalidUrl case, including the
scheme check and the embedded-credentials check in the url validation logic.
Keep the redaction centralized around redact_userinfo and add a regression test
alongside rejects_embedded_credentials that covers a non-http/https URL with
userinfo to ensure no raw secrets appear in errors.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c5e96459-5f90-45f1-a713-e24bf9fcbdb1

📥 Commits

Reviewing files that changed from the base of the PR and between 2d21441 and ad84d34.

📒 Files selected for processing (10)
  • crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs
  • crates/ironclaw_host_runtime/src/memory_provider.rs
  • crates/ironclaw_memory_mem0/src/error.rs
  • crates/ironclaw_memory_mem0/src/lib.rs
  • crates/ironclaw_memory_mem0/src/service.rs
  • crates/ironclaw_memory_mem0/src/transport.rs
  • crates/ironclaw_memory_mem0/src/url_check.rs
  • crates/ironclaw_reborn_cli/src/runtime/mod.rs
  • crates/ironclaw_reborn_composition/src/memory_provider_factory.rs
  • crates/ironclaw_reborn_config/src/config_file.rs

Comment thread crates/ironclaw_reborn_cli/src/runtime/mod.rs
Comment thread crates/ironclaw_reborn_config/src/config_file.rs Outdated
BenKurrek and others added 2 commits June 25, 2026 23:11
… (off by default)

mem0 is now opt-in at build time, mirroring ironclaw_llm/root-llm-provider: ironclaw_memory_mem0 is an optional dependency enabled by a new `memory-mem0` feature on composition, so a default build carries no mem0 code or its reqwest/rustls transport. The factory's mem0 construction (plus its test seam, tests, and the swap integration test) are #[cfg(feature = "memory-mem0")]; a mem0 binding fails closed when the feature is not compiled in. The architecture boundary test asserts the gating (mirroring root-llm-provider), and CI runs the composition suite with the feature so the mem0 tests still execute (feature-off stays covered by the --no-default-features composition run in test.yml).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… schemas, secrets)

From CodeRabbit's full re-review of the rebased PR. All low-risk:
- mem0 provider: read fragments now sort by created_at (mem0 list order is not
  chronological) so append-style docs read back in order; a replace write
  (append=false) is rejected as an Unsupported operation instead of silently
  becoming an add that misreports append:true; check_base_url fails closed on
  hosted mem0 cloud hosts (mem0.ai / *.mem0.ai), enforcing self-hosted-OSS-only;
  the InvalidUrl error no longer echoes the configured URL (drops host/query),
  keeping only the cause.
- reborn_config: mem0_base_url is validated non-empty + trimmed (check_non_empty_trimmed).
- native memory schemas: tree.input rejects absolute / .. / backslash paths (fail
  closed, empty root still allowed); document-read.output requires word_count;
  search.input requires a non-empty query and forbids conflicting aliases (oneOf).
- docs: memory-profiles.md non-goals updated (mem0 provider now exists, off by
  default, feature-gated); host_port.rs docstring no longer over-claims native
  backing (native memory is filesystem-backed, declares no host ports).
- memory_binding: regression test locking that a case variant of the native id
  fails closed (ExtensionId grammar is lowercase-only) rather than misclassifying.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@BenKurrek

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/ironclaw_memory_mem0/src/service.rs (1)

266-276: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Fail loud on malformed mem0 list bodies before rebuilding state.

response_items() is documented as tolerating unrecognized 2xx shapes as “no results”. In read/tree/profile_read that becomes false empty state; in profile_set it can merge over an empty profile and write a new blob that drops existing fields. Use a strict list/profile parser for these paths and map malformed 2xx bodies to an operation error. As per path instructions, **/*.rs: “Fail loud: flag silent-failure patterns … Errors propagate with ? into thiserror types with context.”

Also applies to: 303-313, 331-347, 370-372

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_memory_mem0/src/service.rs` around lines 266 - 276, The
read/profile parsing path is treating malformed 2xx mem0 list bodies as empty
results, which can silently rebuild bad state; update the affected logic in
service.rs around the response_items-based handling in
read/tree/profile_read/profile_set to use a strict parser for list/profile
bodies instead of defaulting to no results. Map any unrecognized or malformed
2xx response shape to an operation error, and ensure the error propagates with ?
through the MemoryServiceError flow so the caller sees a failure rather than an
empty profile or overwritten blob.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/ironclaw_memory_mem0/src/service.rs`:
- Around line 266-276: The read/profile parsing path is treating malformed 2xx
mem0 list bodies as empty results, which can silently rebuild bad state; update
the affected logic in service.rs around the response_items-based handling in
read/tree/profile_read/profile_set to use a strict parser for list/profile
bodies instead of defaulting to no results. Map any unrecognized or malformed
2xx response shape to an operation error, and ensure the error propagates with ?
through the MemoryServiceError flow so the caller sees a failure rather than an
empty profile or overwritten blob.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b5691f37-c643-4680-a1c0-8dcad30d064b

📥 Commits

Reviewing files that changed from the base of the PR and between b1da109 and 3ea70cf.

📒 Files selected for processing (11)
  • crates/ironclaw_host_api/src/host_port.rs
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.input.v1.json
  • crates/ironclaw_host_runtime/src/memory_binding.rs
  • crates/ironclaw_host_runtime/tests/memory_native_schema_validation.rs
  • crates/ironclaw_memory_mem0/src/error.rs
  • crates/ironclaw_memory_mem0/src/service.rs
  • crates/ironclaw_memory_mem0/src/url_check.rs
  • crates/ironclaw_reborn_config/src/config_file.rs
  • docs/reborn/contracts/memory-profiles.md

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

♻️ Duplicate comments (1)
crates/ironclaw_host_runtime/tests/memory_prompt_context.rs (1)

126-172: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add caller-level agent/project mismatch coverage.

This suite proves tenant/user filtering end to end, but the host gate also rejects mismatched agent_id and project_id. That caller-path regression is still untested here, so those scope arms can drift while the helper-level checks stay green.

As per path instructions, "Test through the caller: when a helper gates a side effect, require a test driving the real call site, not only the helper," and as per coding guidelines, "For cross-layer or user-visible Reborn behavior, add a caller-level or whole-path contract test that drives a representative input through the real boundary where the side effect happens."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_host_runtime/tests/memory_prompt_context.rs` around lines 126
- 172, The end-to-end test currently covers tenant/user scope filtering but not
caller-level agent_id and project_id mismatches. Extend the real
load_memory_snippets path in memory_prompt_context.rs using make_service and
MockMemoryService to add separate cases where the provider returns snippets with
the wrong agent_id and wrong project_id, then assert the host drops those
snippets while still keeping an in-scope one. Reference the existing
provider_supplied_cross_scope_snippets_are_dropped_by_the_host test,
MemoryServiceContextSnippet, and test_request so the new coverage exercises the
same admit pipeline instead of only helper-level checks.

Sources: Coding guidelines, Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_host_runtime/assets/memory_native/manifest.toml`:
- Line 5: The package description in the manifest overstates the contract by
implying all model-facing memory tools implement memory.document_store.v1, but
only the read/write tools actually do. Narrow the description to just read and
write in the memory_native manifest so it matches the explicit implements
declarations and does not promise a cross-provider guarantee for search or tree.

In
`@crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/write.md`:
- Around line 1-4: The write prompt is still advertising replace-style behavior,
which conflicts with the supported tool semantics. Update the instructions in
write.md for the memory write tool to mention only supported create/append/patch
actions via target, append, and old_string/new_string, and remove any wording
that implies a replace or append=false mode can be used. Keep the guidance
aligned with the memory-native prompt so the model avoids selecting unsupported
write behavior.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.json`:
- Around line 8-12: The `target` field in the document-write input schema
currently allows any string, so it can accept absolute, traversal, or
backslash-based paths instead of only reserved names or safe relative memory
document paths. Tighten the schema for the `target` property in
`memory/document-write.input.v1.json` to fail closed at validation time by
restricting it to the documented reserved values or a safe relative-path
pattern, and exclude unsafe path forms. Keep the existing `description` and
`default`, but make the `target` contract explicit so invalid paths are rejected
before downstream handling.

In `@crates/ironclaw_host_runtime/src/extension_contracts.rs`:
- Around line 38-40: Soften the cross-layer guarantee in the comment on the
memory ports in extension_contracts.rs: the current wording implies
host.storage.sql_transaction.first_party and host.events.audit definitively back
ironclaw.memory.native, but memory_native_extension.rs treats the native
manifest as filesystem-backed and does not declare host ports. Update the
comment near the extension contract to describe this as intended/expected wiring
for the bundled native memory path rather than an enforced guarantee, and keep
the reference to issue `#3537` if helpful for context.

In `@crates/ironclaw_memory_mem0/src/service.rs`:
- Around line 143-145: The `profile_set` flow is treating any empty result from
`response_items()` as a valid empty profile, which can silently mask malformed
mem0 list responses and overwrite existing fields. Update the logic around
`latest_profile_text()` and `response_items()` so that a recognized empty list
still returns `Map::new()`, but an unrecognized successful response envelope is
surfaced as an error instead of being treated as “no prior profile.”

In `@crates/ironclaw_memory_mem0/src/transport.rs`:
- Around line 79-81: The transport error doc comment in `Mem0TransportError` is
too strong for the current behavior of `execute()`, since non-JSON bodies are
converted to `Value::Null` instead of surfacing a transport failure. Update the
docs near `Mem0TransportError` and the `execute()` path to either describe the
fallback behavior as intentional or change the parsing flow so non-JSON
responses actually raise `Mem0TransportError`; make sure the wording matches the
real contract.

In `@crates/ironclaw_memory/src/service.rs`:
- Around line 74-85: Validate and fail closed on document targets before any
provider-specific dispatch. Move the scoped-relative containment check out of
the native-only path and into the shared parser/helper used by the service
entrypoints so `write`, `read`, and `tree` all reject non-scoped, local, or
traversal-style targets consistently even when `memory.document_store.v1` is
rebound to mem0. Keep the target validation alongside the existing
`input.get("target")` parsing in `service.rs`, and ensure both provider adapters
consume only already-validated targets.

---

Duplicate comments:
In `@crates/ironclaw_host_runtime/tests/memory_prompt_context.rs`:
- Around line 126-172: The end-to-end test currently covers tenant/user scope
filtering but not caller-level agent_id and project_id mismatches. Extend the
real load_memory_snippets path in memory_prompt_context.rs using make_service
and MockMemoryService to add separate cases where the provider returns snippets
with the wrong agent_id and wrong project_id, then assert the host drops those
snippets while still keeping an in-scope one. Reference the existing
provider_supplied_cross_scope_snippets_are_dropped_by_the_host test,
MemoryServiceContextSnippet, and test_request so the new coverage exercises the
same admit pipeline instead of only helper-level checks.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5b0df488-9b6d-4429-a9bd-575c8d3ff9d0

📥 Commits

Reviewing files that changed from the base of the PR and between e67882e and 3ea70cf.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock
📒 Files selected for processing (67)
  • Cargo.toml
  • crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs
  • crates/ironclaw_host_api/src/host_port.rs
  • crates/ironclaw_host_runtime/assets/memory_native/manifest.toml
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/read.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/search.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/tree.md
  • crates/ironclaw_host_runtime/assets/memory_native/prompts/memory-native/write.md
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-read.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/search.output.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.input.v1.json
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/tree.output.v1.json
  • crates/ironclaw_host_runtime/src/extension_contracts.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/memory.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/mod.rs
  • crates/ironclaw_host_runtime/src/first_party_tools/schemas.rs
  • crates/ironclaw_host_runtime/src/lib.rs
  • crates/ironclaw_host_runtime/src/memory_binding.rs
  • crates/ironclaw_host_runtime/src/memory_context.rs
  • crates/ironclaw_host_runtime/src/memory_native_extension.rs
  • crates/ironclaw_host_runtime/src/memory_profiles.rs
  • crates/ironclaw_host_runtime/src/memory_provider.rs
  • crates/ironclaw_host_runtime/src/surface.rs
  • crates/ironclaw_host_runtime/src/user_profile_source.rs
  • crates/ironclaw_host_runtime/tests/first_party_builtin_tools.rs
  • crates/ironclaw_host_runtime/tests/memory_capability_profiles.rs
  • crates/ironclaw_host_runtime/tests/memory_native_manifest.rs
  • crates/ironclaw_host_runtime/tests/memory_native_schema_validation.rs
  • crates/ironclaw_host_runtime/tests/memory_prompt_context.rs
  • crates/ironclaw_host_runtime/tests/user_profile_roundtrip.rs
  • crates/ironclaw_memory/src/lib.rs
  • crates/ironclaw_memory/src/service.rs
  • crates/ironclaw_memory_mem0/Cargo.toml
  • crates/ironclaw_memory_mem0/src/config.rs
  • crates/ironclaw_memory_mem0/src/error.rs
  • crates/ironclaw_memory_mem0/src/lib.rs
  • crates/ironclaw_memory_mem0/src/service.rs
  • crates/ironclaw_memory_mem0/src/transport.rs
  • crates/ironclaw_memory_mem0/src/url_check.rs
  • crates/ironclaw_memory_mem0/tests/live_local_mem0.rs
  • crates/ironclaw_memory_native/src/service.rs
  • crates/ironclaw_memory_native/tests/memory_service_facade.rs
  • crates/ironclaw_reborn_cli/src/runtime/mod.rs
  • crates/ironclaw_reborn_composition/Cargo.toml
  • crates/ironclaw_reborn_composition/src/factory.rs
  • crates/ironclaw_reborn_composition/src/input.rs
  • crates/ironclaw_reborn_composition/src/lib.rs
  • crates/ironclaw_reborn_composition/src/local_dev_capability_policy.toml
  • crates/ironclaw_reborn_composition/src/memory_binding.rs
  • crates/ironclaw_reborn_composition/src/memory_provider_factory.rs
  • crates/ironclaw_reborn_composition/src/projection/display_preview.rs
  • crates/ironclaw_reborn_composition/src/projection/tests/display_preview.rs
  • crates/ironclaw_reborn_composition/src/runtime.rs
  • crates/ironclaw_reborn_composition/src/runtime/local_dev.rs
  • crates/ironclaw_reborn_composition/tests/memory_mem0_swap.rs
  • crates/ironclaw_reborn_config/src/config_file.rs
  • crates/ironclaw_reborn_config/src/lib.rs
  • docs/adr/0001-capability-manifest-v2-hard-cutover.md
  • docs/adr/0002-native-memory-uses-host-storage-ports.md
  • docs/reborn/contracts/memory-profiles.md
  • scripts/ci/package-feature-flags.sh
  • tests/reborn_trace_first_party_tool_coverage.rs
  • tests/support/reborn/harness.rs

Comment thread crates/ironclaw_host_runtime/assets/memory_native/manifest.toml Outdated
Comment thread crates/ironclaw_host_runtime/src/extension_contracts.rs Outdated
Comment thread crates/ironclaw_memory_mem0/src/service.rs Outdated
Comment thread crates/ironclaw_memory_mem0/src/transport.rs Outdated
Comment thread crates/ironclaw_memory/src/service.rs
…, fail-loud, docs)

From CodeRabbit's fresh full re-review. Most notable: the native memory JSON
input schemas are model-facing (advertised in parameters_schema) and are NOT
host-validated against actual tool arguments before dispatch, so a traversal
target could reach a provider verbatim. Added a provider-neutral
reject_out_of_scope_target guard in MemoryServiceWriteRequest::from_tool_input
(mirrors the schema pattern) so every bound provider -- including mem0, which
stores the target verbatim -- gets containment, not just native.

Also:
- document-write.input schema rejects absolute / .. / backslash targets (fail
  closed, matching the sibling schemas).
- mem0 response_items fails loud (UnrecognizedResponse) on an unrecognized 2xx
  body instead of silently returning empty (which let a malformed list response
  overwrite existing profile fields).
- docs: manifest description scoped (search/tree implement no portable profile);
  extension_contracts + transport docstrings corrected (native is filesystem-
  backed / declares no host ports; non-JSON bodies degrade to Null, not an error).

The mem0 replace-write rejection is provider-specific (mem0 OSS is append-only);
native still supports replace, so the shared write prompt is left unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/ironclaw_memory_mem0/src/error.rs (1)

37-40: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Update the Api docs to match fail-loud body parsing.

UnrecognizedResponse now makes unrecognized 2xx list/search bodies fail loud, but Lines 37-40 still promise they degrade to “no results.” That cross-layer guarantee is stale. Repo invariant: comments that promise guarantees across layers must be enforced or softened.

Suggested doc fix
-    /// parsing is deliberately tolerant (an unrecognized search/list shape
-    /// degrades to "no results"), so a malformed-but-2xx body is not an error;
-    /// only the HTTP status is.
+    /// body-shape validation is handled separately: unrecognized successful
+    /// list/search bodies surface as `UnrecognizedResponse`, so this variant is
+    /// only for HTTP status failures.

Also applies to: 64-72

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_memory_mem0/src/error.rs` around lines 37 - 40, The Api error
docs are now stale: they still promise unrecognized list/search bodies degrade
to “no results,” but UnrecognizedResponse makes those 2xx bodies fail loud.
Update the doc comments in error.rs for Api and the related list/search behavior
to describe the new strict parsing behavior, or soften the guarantee so it no
longer claims tolerant fallback.

Source: Coding guidelines

♻️ Duplicate comments (1)
crates/ironclaw_memory_mem0/src/transport.rs (1)

83-86: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Don’t claim the service tolerates null/unrecognized bodies.

execute() may degrade non-JSON to Value::Null, but response_items() now maps null/unrecognized list bodies to Mem0Error::UnrecognizedResponse. Say this is transport-only behavior. Repo invariant: comments that promise guarantees across layers must be enforced or softened.

Suggested doc fix
-/// `execute` degrades it to `Value::Null` (the HTTP status is the authoritative
-/// success signal and the service tolerates a null/unrecognized body), so an
-/// un-parseable body never surfaces here.
+/// `execute` degrades it to `Value::Null` (the HTTP status is the authoritative
+/// success signal at the transport layer). Service methods that require a list
+/// shape validate null/unrecognized bodies and fail loud with
+/// `Mem0Error::UnrecognizedResponse`; those failures do not surface here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_memory_mem0/src/transport.rs` around lines 83 - 86, The doc
comment in transport.rs is overstating the cross-layer behavior by claiming the
service tolerates null or unrecognized bodies. Update the comment near the
transport response handling to describe only transport-level behavior:
`execute()` may downgrade non-JSON responses to `Value::Null`, but higher-level
parsing in `response_items()` can still turn null/unrecognized list bodies into
`Mem0Error::UnrecognizedResponse`. Keep the wording aligned with the actual
behavior of `execute` and `response_items`.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.json`:
- Line 10: The target field description in the document-write schema is too
native-specific because it claims all writes are resolved under the scoped
/memory mount, which is only true in the native binding. Update the schema text
in the document-write input definition to keep it backend-neutral, preserving
the validation rules for reserved names and relative paths while removing or
generalizing the /memory filesystem guarantee so it also fits the mem0-backed
flow.

In `@crates/ironclaw_memory/src/service.rs`:
- Around line 553-564: Reject empty and whitespace-only targets in the shared
containment guard so the provider-neutral check fails closed for blank input.
Update reject_out_of_scope_target in service.rs to treat "" and "   " as invalid
alongside absolute paths, traversal, and backslashes, and ensure the mem0
adapter cannot pass such values through unchanged. Add regression tests covering
blank target cases in the relevant shared validation path and adapter-facing
behavior.

---

Outside diff comments:
In `@crates/ironclaw_memory_mem0/src/error.rs`:
- Around line 37-40: The Api error docs are now stale: they still promise
unrecognized list/search bodies degrade to “no results,” but
UnrecognizedResponse makes those 2xx bodies fail loud. Update the doc comments
in error.rs for Api and the related list/search behavior to describe the new
strict parsing behavior, or soften the guarantee so it no longer claims tolerant
fallback.

---

Duplicate comments:
In `@crates/ironclaw_memory_mem0/src/transport.rs`:
- Around line 83-86: The doc comment in transport.rs is overstating the
cross-layer behavior by claiming the service tolerates null or unrecognized
bodies. Update the comment near the transport response handling to describe only
transport-level behavior: `execute()` may downgrade non-JSON responses to
`Value::Null`, but higher-level parsing in `response_items()` can still turn
null/unrecognized list bodies into `Mem0Error::UnrecognizedResponse`. Keep the
wording aligned with the actual behavior of `execute` and `response_items`.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 5820d74d-fd61-4090-9f21-56a947d07cf4

📥 Commits

Reviewing files that changed from the base of the PR and between 3ea70cf and bb40e02.

📒 Files selected for processing (8)
  • crates/ironclaw_host_runtime/assets/memory_native/manifest.toml
  • crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.json
  • crates/ironclaw_host_runtime/src/extension_contracts.rs
  • crates/ironclaw_host_runtime/tests/memory_native_schema_validation.rs
  • crates/ironclaw_memory/src/service.rs
  • crates/ironclaw_memory_mem0/src/error.rs
  • crates/ironclaw_memory_mem0/src/service.rs
  • crates/ironclaw_memory_mem0/src/transport.rs

},
"target": {
"type": "string",
"description": "Where to write: 'memory' for MEMORY.md, 'daily_log' for today's log, 'heartbeat' for HEARTBEAT.md checklist, 'bootstrap' to clear BOOTSTRAP.md (content is ignored; the file is always cleared), or a relative memory document path. A reserved name or relative path is allowed; it must not be absolute (no leading '/'), contain '..' traversal, or use backslash separators. The host filesystem additionally resolves the path within the scoped /memory mount.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Keep the target description backend-neutral.

This schema is surfaced on the shared document-write tool, but the last sentence is only true for the native filesystem binding. When the profile is rebound to mem0, target is stored verbatim as metadata rather than resolved under /memory, so the description currently overstates a native-only guarantee.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@crates/ironclaw_host_runtime/assets/memory_native/schemas/memory/document-write.input.v1.json`
at line 10, The target field description in the document-write schema is too
native-specific because it claims all writes are resolved under the scoped
/memory mount, which is only true in the native binding. Update the schema text
in the document-write input definition to keep it backend-neutral, preserving
the validation rules for reserved names and relative paths while removing or
generalizing the /memory filesystem guarantee so it also fits the mem0-backed
flow.

Comment on lines +553 to +564
/// Reject a write `target` that would escape the scoped memory mount. Mirrors the
/// fail-closed `not` pattern the model-facing `document-write` input schema
/// advertises — `(^/)|(\.\.)|(\\)`: an absolute path (leading `/`), any `..`
/// traversal, or a backslash separator. Reserved names (`daily_log`, `memory`,
/// `heartbeat`, `bootstrap`) and ordinary relative document paths (`notes/x.md`)
/// pass unchanged.
fn reject_out_of_scope_target(target: &str) -> Result<(), MemoryServiceError> {
if target.starts_with('/') || target.contains("..") || target.contains('\\') {
return Err(MemoryServiceError::input());
}
Ok(())
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Reject empty/blank targets in the shared guard.

The new provider-neutral check still accepts "" and " ". That breaks the documented contract (“reserved name or relative path”) and, on the mem0 binding, persists an empty target metadata tag because the adapter uses the value verbatim. Fail closed here and add matching regression cases.

Suggested fix
 fn reject_out_of_scope_target(target: &str) -> Result<(), MemoryServiceError> {
-    if target.starts_with('/') || target.contains("..") || target.contains('\\') {
+    if target.trim().is_empty()
+        || target.starts_with('/')
+        || target.contains("..")
+        || target.contains('\\')
+    {
         return Err(MemoryServiceError::input());
     }
     Ok(())
 }
@@
-        for target in ["/abs", "../escape", "notes/../secrets", "notes\\evil"] {
+        for target in ["", "   ", "/abs", "../escape", "notes/../secrets", "notes\\evil"] {
             let input = json!({ "target": target, "content": "x" });
             let result = MemoryServiceWriteRequest::from_tool_input(&input);

As per coding guidelines, "Fail closed for auth, approvals, trust, filesystem containment, network policy, secret leases, runtime selection, and adapter identity."

Also applies to: 617-657

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_memory/src/service.rs` around lines 553 - 564, Reject empty
and whitespace-only targets in the shared containment guard so the
provider-neutral check fails closed for blank input. Update
reject_out_of_scope_target in service.rs to treat "" and "   " as invalid
alongside absolute paths, traversal, and backslashes, and ensure the mem0
adapter cannot pass such values through unchanged. Add regression tests covering
blank target cases in the relevant shared validation path and adapter-facing
behavior.

Source: Coding guidelines

BenKurrek and others added 2 commits June 26, 2026 07:56
Completes the mem0 feature-gate: ironclaw_reborn_cli now exposes a memory-mem0 feature that enables ironclaw_reborn_composition/memory-mem0, so an ironclaw-reborn binary built with --features memory-mem0 can bind memory.document_store.v1 to a self-hosted mem0 server. Without it the feature was only reachable on the composition crate, never the actual binary. Mirrors the existing root-llm-provider / webui-v2-beta forwarding.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…llowups

# Conflicts:
#	crates/ironclaw_reborn_cli/Cargo.toml

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs`:
- Around line 700-703: The boundary scan only covers crates under
collect_provider_tool_name_boundary_uses on workspace crates, so root-crate code
in src/ can bypass the ProviderToolName/provider_tool_name guardrail. Update the
test setup in reborn_dependency_boundaries.rs to scan both the crates/ tree and
the top-level src/ tree from workspace_root(), and keep any necessary allowlist
exceptions for legitimate provider boundary uses found there.

In `@crates/ironclaw_reborn_composition/src/memory_binding.rs`:
- Around line 27-33: The comment in the memory binding match arm overstates the
trust guarantee for RebornCompositionProfile::HostedSingleTenantVolume by
claiming it shares the same “single-tenant trust model” as HostedSingleTenant,
even though the runtime resolves it as DeploymentMode::HostedMultiTenant. Update
the comment near the HostedSingleTenant / HostedSingleTenantVolume mapping in
memory_binding.rs to either soften it to describe intended classification only,
or, if the stronger claim is intended, enforce it with matching policy/tests
elsewhere; keep the actual MemoryDeploymentProfile::HostedSingleTenant mapping
unchanged unless the policy is being corrected too.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4e047232-44b2-4e2d-b353-77c5345a46fc

📥 Commits

Reviewing files that changed from the base of the PR and between 4c96023 and e58aa9e.

📒 Files selected for processing (11)
  • crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs
  • crates/ironclaw_reborn_cli/Cargo.toml
  • crates/ironclaw_reborn_cli/src/runtime/mod.rs
  • crates/ironclaw_reborn_composition/src/factory.rs
  • crates/ironclaw_reborn_composition/src/input.rs
  • crates/ironclaw_reborn_composition/src/lib.rs
  • crates/ironclaw_reborn_composition/src/memory_binding.rs
  • crates/ironclaw_reborn_composition/src/runtime.rs
  • crates/ironclaw_reborn_composition/src/runtime/local_dev.rs
  • crates/ironclaw_reborn_config/src/config_file.rs
  • tests/support/reborn/harness.rs

…ded DoS)

New advisory on lopdf (via pdf-extract in ironclaw_extractors, PDF attachment extraction) with no patched release in our semver range yet. Bounded DoS only -- a crash of the extraction task on a hostile user-supplied PDF, not RCE and not the whole process. Matches the existing deny.toml ignore pattern; remove once lopdf/pdf-extract ship a fixed release. Unrelated to the memory work; surfaced because the main merge re-ran cargo-deny.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs`:
- Around line 700-703: The boundary scan only covers crates under
collect_provider_tool_name_boundary_uses on workspace crates, so root-crate code
in src/ can bypass the ProviderToolName/provider_tool_name guardrail. Update the
test setup in reborn_dependency_boundaries.rs to scan both the crates/ tree and
the top-level src/ tree from workspace_root(), and keep any necessary allowlist
exceptions for legitimate provider boundary uses found there.

In `@crates/ironclaw_reborn_composition/src/memory_binding.rs`:
- Around line 27-33: The comment in the memory binding match arm overstates the
trust guarantee for RebornCompositionProfile::HostedSingleTenantVolume by
claiming it shares the same “single-tenant trust model” as HostedSingleTenant,
even though the runtime resolves it as DeploymentMode::HostedMultiTenant. Update
the comment near the HostedSingleTenant / HostedSingleTenantVolume mapping in
memory_binding.rs to either soften it to describe intended classification only,
or, if the stronger claim is intended, enforce it with matching policy/tests
elsewhere; keep the actual MemoryDeploymentProfile::HostedSingleTenant mapping
unchanged unless the policy is being corrected too.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4e047232-44b2-4e2d-b353-77c5345a46fc

📥 Commits

Reviewing files that changed from the base of the PR and between 4c96023 and e58aa9e.

📒 Files selected for processing (11)
  • crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs
  • crates/ironclaw_reborn_cli/Cargo.toml
  • crates/ironclaw_reborn_cli/src/runtime/mod.rs
  • crates/ironclaw_reborn_composition/src/factory.rs
  • crates/ironclaw_reborn_composition/src/input.rs
  • crates/ironclaw_reborn_composition/src/lib.rs
  • crates/ironclaw_reborn_composition/src/memory_binding.rs
  • crates/ironclaw_reborn_composition/src/runtime.rs
  • crates/ironclaw_reborn_composition/src/runtime/local_dev.rs
  • crates/ironclaw_reborn_config/src/config_file.rs
  • tests/support/reborn/harness.rs
🛑 Comments failed to post (2)
crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs (1)

700-703: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Scan the root crate, not only workspace crates.

Line 702 only walks crates/, so any ProviderToolName / provider_tool_name use in top-level src/ routing, channels, or legacy root-crate code bypasses this guardrail. Add src/ to the scanned roots, keeping explicit allowlist entries for any legitimate provider boundary there.

Proposed fix
     let root = workspace_root();
     let mut uses = Vec::new();
-    collect_provider_tool_name_boundary_uses(&root.join("crates"), &root, &mut uses);
+    for source_root in [root.join("crates"), root.join("src")] {
+        if source_root.exists() {
+            collect_provider_tool_name_boundary_uses(&source_root, &root, &mut uses);
+        }
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    let root = workspace_root();
    let mut uses = Vec::new();
    for source_root in [root.join("crates"), root.join("src")] {
        if source_root.exists() {
            collect_provider_tool_name_boundary_uses(&source_root, &root, &mut uses);
        }
    }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_architecture/tests/reborn_dependency_boundaries.rs` around
lines 700 - 703, The boundary scan only covers crates under
collect_provider_tool_name_boundary_uses on workspace crates, so root-crate code
in src/ can bypass the ProviderToolName/provider_tool_name guardrail. Update the
test setup in reborn_dependency_boundaries.rs to scan both the crates/ tree and
the top-level src/ tree from workspace_root(), and keep any necessary allowlist
exceptions for legitimate provider boundary uses found there.
crates/ironclaw_reborn_composition/src/memory_binding.rs (1)

27-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Soften or enforce the hosted-volume trust-model comment.

The code classifies HostedSingleTenantVolume as hosted-single-tenant memory, but the runtime policy context resolves this profile as DeploymentMode::HostedMultiTenant. Keep the mapping if intended, but don’t claim the same “single-tenant trust model” unless that is enforced elsewhere.

As per coding guidelines, “Comments that promise guarantees across layers must either be enforced by code/tests or softened to describe intent.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/ironclaw_reborn_composition/src/memory_binding.rs` around lines 27 -
33, The comment in the memory binding match arm overstates the trust guarantee
for RebornCompositionProfile::HostedSingleTenantVolume by claiming it shares the
same “single-tenant trust model” as HostedSingleTenant, even though the runtime
resolves it as DeploymentMode::HostedMultiTenant. Update the comment near the
HostedSingleTenant / HostedSingleTenantVolume mapping in memory_binding.rs to
either soften it to describe intended classification only, or, if the stronger
claim is intended, enforce it with matching policy/tests elsewhere; keep the
actual MemoryDeploymentProfile::HostedSingleTenant mapping unchanged unless the
policy is being corrected too.

Source: Coding guidelines

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@deny.toml`:
- Around line 30-35: The RUSTSEC-2026-0187 exception in deny.toml is missing a
self-expiring marker, so update the surrounding advisory comment to include a
concrete revisit date and a tracking issue or PR reference like the nearby
RUSTSEC-2026-0097 entry. Keep the exception scoped to this specific advisory key
and make the comment explicit enough that CI reviewers can see when and where it
must be re-evaluated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: acfb174f-c50d-4d52-9fec-f697a67752ce

📥 Commits

Reviewing files that changed from the base of the PR and between e58aa9e and f8377b9.

📒 Files selected for processing (1)
  • deny.toml

Comment thread deny.toml
Comment on lines +30 to +35
# lopdf stack overflow on deeply-nested PDF objects, reached via pdf-extract
# in ironclaw_extractors (PDF attachment text extraction). New advisory with
# no patched lopdf release in our semver range yet. Bounded DoS only — a crash
# of the extraction task on a hostile user-supplied PDF, not RCE and not the
# whole process. Remove once lopdf / pdf-extract ship a fixed release.
"RUSTSEC-2026-0187",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Make this ignore self-expiring.

Unlike the nearby RUSTSEC-2026-0097 exception, this one has no concrete review date or issue reference. Because both CI entrypoints load deny.toml directly, this suppresses the advisory repo-wide until someone manually rediscovers it. Add a revisit date and tracking issue/PR to the comment so the exception cannot drift into a permanent blind spot.

As per coding guidelines, "If the bug is non-trivial, security-sensitive, or changes behavior across subsystems, open or confirm an issue first so the approach can be aligned before implementation."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@deny.toml` around lines 30 - 35, The RUSTSEC-2026-0187 exception in deny.toml
is missing a self-expiring marker, so update the surrounding advisory comment to
include a concrete revisit date and a tracking issue or PR reference like the
nearby RUSTSEC-2026-0097 entry. Keep the exception scoped to this specific
advisory key and make the comment explicit enough that CI reviewers can see when
and where it must be re-evaluated.

Source: Coding guidelines

…e + scope into user_id)

Local-dev native memory is isolated per workspace (its filesystem store lives under
local_dev_root); mem0 (a shared server) was not, since the local-dev runtime uses a
fixed scope. mem0 OSS enforces search/get_all filtering by user_id (and agent_id) but
NOT by app_id -- a top-level app_id is accepted yet silently ignored when filtering
(verified empirically: cross-app_id queries leak). So we encode the entire partition
into the one key guaranteed enforced:

- The provider folds the workspace partition (config.app_id, set per-workspace by the
  local-dev composition from local_dev_root) into the user_id namespace at all 7
  namespace sites (search/write/read/tree/profile-read/profile-set/retrieve_context).
  app_id is still stamped as forward-compat metadata but is NOT relied on for isolation.
- The local-dev composition derives a per-workspace app_id from the canonical local-dev
  root; production is untouched (no app_id -> pure scope namespace, so memory persists
  across restarts for the same scope).

Result: local-dev mem0 partitions like native (workspace x tenant/user/agent/project).
Verified by a live cross-isolation test (cross-workspace AND cross-scope both return
zero on search and list) + a regression guard locking the user_id prefix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor: experienced 6-19 merged PRs risk: medium Business logic, config, or moderate-risk modules scope: dependencies Dependency updates scope: docs Documentation size: XL 500+ changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reborn: model memory as a userland extension

1 participant